[webkit-reviews] review granted: [Bug 134677] [ftlopt] DFG should be able to do GCSE in SSA and this should be unified with the CSE in CPS, and both of these things should use abstract heaps for reasoning about effects : [Attachment 234773] the patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 134677: [ftlopt] DFG should be able to do GCSE in SSA and this should be
unified with the CSE in CPS, and both of these things should use abstract heaps
for reasoning about effects
https://bugs.webkit.org/show_bug.cgi?id=134677

Attachment 234773: the patch
https://bugs.webkit.org/attachment.cgi?id=234773&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=234773&action=review


> Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:71
> +    ClobberFilter filter(heap);
> +    map.removeIf(filter);

Instead of using a functor, you could use a lambda:

map.removeIf([heap](const ImpureMap::KeyValuePairType& pair) {	return
heap.overlaps(pair.key.heap()); }); or some such jazz.

> Source/JavaScriptCore/dfg/DFGGraph.h:667
> +    void getBlocksInPreOrder(Vector<BasicBlock*>& result);
> +    void getBlocksInPostOrder(Vector<BasicBlock*>& result);

There isn't any reason I can see that these couldn't return a Vector, rather
than taking one by reference.  RVO should kick in for them.

> Source/WTF/wtf/HashTable.h:1037
> +    template<typename Functor>
> +    inline void HashTable<Key, Value, Extractor, HashFunctions, Traits,
KeyTraits>::removeIf(Functor& functor)

I really like this, and was just telling Tim Horton we needed this the other
day.  If you wanted to be awesome, you would land this separate (and in trunk).