[PATCH] D37121: [DivRemHoist] add a pass to move div/rem pairs into the same block (PR31028)

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added inline comments.


================
Comment at: lib/Transforms/Scalar/DivRemHoist.cpp:78
+
+    if (DT.dominates(DivInst, RemInst)) {
+      // Hoist the rem into the div block if the implicit speculated
----------------
You should check dominance of the parents, not the instructions. Though you currently avoid it, dt->dominates is a linear time test if you use it on instructions (and that's not obvious here) in the same block.

The recommendation is to use orderedinstructions instead if you need to check same-block dominance, but you don't :)





================
Comment at: lib/Transforms/Scalar/DivRemHoist.cpp:83
+        continue;
+      RemInst->moveBefore(DivInst);
+      NumHoisted++;
----------------
It should go after, to retain the relative ordering.


================
Comment at: lib/Transforms/Scalar/DivRemHoist.cpp:89
+      // because division is an implicit component of remainder.
+      DivInst->moveBefore(RemInst);
+      NumHoisted++;
----------------
Ditto, this should be after.


https://reviews.llvm.org/D37121