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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Transforms/Scalar/DivRemHoist.cpp:50
+  // operands and opcode (signed or unsigned).
+  DenseMap<DivRemMapKey, Instruction *> DivMap, RemMap;
+  for (auto &BB : F) {
----------------
I'm missing something: where is DivRemMapKey defined?


================
Comment at: lib/Transforms/Scalar/DivRemHoist.cpp:85
+      // because division is an implicit component of remainder. The backend
+      // can do whatever is optimal for the div/rem pair.
+      DivInst->moveAfter(RemInst);
----------------
I don't understand what's going on here? Can you please explain in the comment. Why is this way cheap but hoisting the rem near the div is not? I'm guessing this has something to do with the fact that DAGCombiner::useDivRem isn't called for divisions if TLI.isIntDivCheap returns true, but is always called on remainders? If that's it, should we mirror that logic more directly here (i.e. somehow directly incorporate the result of calling TLI.isIntDivCheap)?


https://reviews.llvm.org/D37121