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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/Scalar/DivRemHoist.cpp:84
+      // multiplication is not too costly.
+      if (TTI.getArithmeticInstrCost(Instruction::Mul, RemInst->getType()) > 1)
+        continue;
----------------
hfinkel wrote:
> Please don't hard-code 1 here as the basis for comparison to the cost model. The normalization is target specific. Feel free to compare it a corresponding cost for Instruction::Add, for example.
> 
> Also, to be clear, you're using the reciprocal-throughput model here. So if we can perform two adds every cycle, and that has a cost of 1, and we can do only one multiple per cycle, it would have a cost of 2. Is that too much?
> 
> You might want to use the user-cost model (which is more like code size in some sense), but that's what SimplyCFG uses to do speculation. You could call TTTI->getOperationCost and compare it to TargetTransformInfo::TCC_Basic. We'll need to clean all of this up at some point, but I think that would be akin to what SimplifyCFG is doing for speculation costs.
Ah, right. I should've remembered all that since I've complained about it before!
Let me take another look at the current state of those cost models and do something less obnoxious than a magic number.


https://reviews.llvm.org/D37121