[PATCH] D12742: [ARM] Scaling up values in ARMBaseInstrInfo::isProfitableToIfCvt() before they are scaled by a probability to avoid precision issue.

[Cong Hou via llvm-commits]

congh added inline comments.

================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:1677
@@ +1676,3 @@
+  unsigned UnpredCost = Probability.scale(NumCycles * ScalingUpFactor);
+  UnpredCost += 1 * ScalingUpFactor; // The branch itself
+  UnpredCost += Subtarget.getMispredictionPenalty() * ScalingUpFactor / 10;
----------------
rengolin wrote:
> You don't really need the 1 * here. The comment makes it clear enough.
OK. Thanks!

================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:1701
@@ -1695,1 +1700,3 @@
+  UnpredCost += 1 * ScalingUpFactor; // The branch itself
+  UnpredCost += Subtarget.getMispredictionPenalty() * ScalingUpFactor / 10;
 
----------------
rengolin wrote:
> scale() returns an uint64, so I'm guessing we were never be using more than 22 bits for the UnpredCost.
As long as the input can fit in a 32-bit integer, so can the output.

================
Comment at: test/CodeGen/ARM/2013-10-11-select-stalls.ll:2
@@ -1,3 +1,3 @@
 ; REQUIRES: asserts
-; RUN: llc < %s -mtriple=thumbv7-apple-ios -stats 2>&1 | not grep "Number of pipeline stalls"
+; RUN: llc < %s -mtriple=thumbv7-apple-ios -stats 2>&1 | grep "Number of pipeline stalls"
 ; Evaluate the two vld1.8 instructions in separate MBB's,
----------------
rengolin wrote:
> Is this intentional? I understand that pipeline stalls may be less bad than branch misprediction, but we don't want to have too many either. If this is a side effect of the change, I guess we should have some concrete numbers to rely on.
Previously, the machine code sinking pass moves those two vector loads into different blocks to prevent pipeline stalls by intention. The if converter is scheduled after this pass and with this patch it will if-convert those two vector loads from two blocks to two loads in the same block. This may be a side effect of this patch. Ideally, the if converter should consider the cost of pipeline stall. So I think the solution is improving if converter's cost model to take pipeline stalls into consideration. What do you think?


http://reviews.llvm.org/D12742