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

[Renato Golin via llvm-commits]

rengolin 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;
----------------
You don't really need the 1 * here. The comment makes it clear enough.

================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:1701
@@ -1695,1 +1700,3 @@
+  UnpredCost += 1 * ScalingUpFactor; // The branch itself
+  UnpredCost += Subtarget.getMispredictionPenalty() * ScalingUpFactor / 10;
 
----------------
scale() returns an uint64, so I'm guessing we were never be using more than 22 bits for the UnpredCost.

================
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,
----------------
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.


http://reviews.llvm.org/D12742