[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) {
----------------
spatel wrote:
> hfinkel wrote:
> > I'm missing something: where is DivRemMapKey defined?
> This is an existing struct used for a different div/rem optimization. It lives in:
> #include "llvm/Transforms/Utils/BypassSlowDivision.h"
Ah, okay.


================
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);
----------------
spatel wrote:
> hfinkel wrote:
> > 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)?
> This is independent of anything in the backend. That's why this transform is frustrating - it's half target-independent. :)
> 
> Let's see if some more words or repeating the formula would make it better:
> "Hoist the div into the rem block. No cost calculation is needed because division is an implicit component of remainder:
>  X % Y --> X - ((X / Y) * Y)
> If the target has a unified instruction for div/rem, then this will occur in a single instruction. If the target does not have a unified instruction for div/rem, then it must calculate remainder as (sub X, (mul (div X, Y), Y). Either way, hoisting division will be free."
That is much better, thanks. However, are you making that substitution? It looks like you're just moving the division in this case (i.e. only doing `DivInst->moveAfter(RemInst);`)?


https://reviews.llvm.org/D37121