[PATCH] D28129: NewGVN: Sort Dominator Tree in RPO order, and use that for generating order.

Davide Italiano via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 28 07:04:48 PST 2016


davide accepted this revision.
davide added a comment.
This revision is now accepted and ready to land.

Sorry for the slow response, I'm out('ish) of the office these days. I took a close look at your patch.
I happen to be lucky enough to hit a case in the wild where this already matters.  The number of iteration goes down from hundreds to ~10, which makes compile time/me happier. 
I'll see if I can investigate 
Your reasoning looks good to me, so feel free to check this in (modulo small nits). I'll do some more extensive testing when I'm in back in California early next year (although as you pointed out this is not exposed without predicates handling.



================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1375-1377
+  // The dominator tree does guarantee that, for a given dom tree node, it's
+  // parent must occur before it in the RPO ordering. Thus, we only need to sort
+  // the siblings.
----------------
I apologize in advance if I'm wrong, as I'm not a native speaker, but it shouldn't be 
`its parent` (at least this is the way I've always seen it written down)?
Please ignore otherwise.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1457
   }
-
   Changed |= eliminateInstructions(F);
----------------
Spurious removal of a newline?


https://reviews.llvm.org/D28129





More information about the llvm-commits mailing list