Use BranchProbability::scale() to scale an integer with a probability in ARMBaseInstrInfo.cpp,

[congh]

Summary

Previously in isProfitableToIfCvt() in ARMBaseInstrInfo.cpp, the multiplication between an integer and a branch probability is done manually in an unsafe way that may lead to overflow. This patch corrects those cases by using BranchProbability's member function scale() to avoid overflow (which stores the intermediate result in int64).

[congh]

Ping?

[rengolin]

This looks sane to me. Do you have a code where it fails? Would it be possible to turn into a test?

[congh]

In D12295#233374, @rengolin wrote:

This looks sane to me. Do you have a code where it fails? Would it be possible to turn into a test?

When the probability is represented with a very large numerator, we will get overflow here (this happened to me when I changed the representation of the BranchProbability by using a very large fixed denominator). BranchProbability provides scale() interface just for this case, so using it should be the right way to scale an integer. Is a test necessary for this fix?

[rengolin]

In D12295#233532, @congh wrote:

When the probability is represented with a very large numerator, we will get overflow here (this happened to me when I changed the representation of the BranchProbability by using a very large fixed denominator). BranchProbability provides scale() interface just for this case, so using it should be the right way to scale an integer. Is a test necessary for this fix?

If the problem was found with a test case, then yes. But if it was just you fiddling with internal constants, I think the patch is obvious and should be ok without a test.

[congh]

In D12295#233554, @rengolin wrote:

In D12295#233532, @congh wrote:

When the probability is represented with a very large numerator, we will get overflow here (this happened to me when I changed the representation of the BranchProbability by using a very large fixed denominator). BranchProbability provides scale() interface just for this case, so using it should be the right way to scale an integer. Is a test necessary for this fix?

If the problem was found with a test case, then yes. But if it was just you fiddling with internal constants, I think the patch is obvious and should be ok without a test.

This should be the latter case. Thanks for the review!

lib/Target/ARM/ARMBaseInstrInfo.cpp

if (!Pred->empty()) {
               return false;
           }
         }
       }
     }
   }
 
   // Attempt to estimate the relative costs of predication versus branching.
-  unsigned UnpredCost = Probability.getNumerator() * NumCycles;
-  UnpredCost /= Probability.getDenominator();
+  unsigned UnpredCost = Probability.scale(NumCycles);
   UnpredCost += 1; // The branch itself
   UnpredCost += Subtarget.getMispredictionPenalty() / 10;
 
   return (NumCycles + ExtraPredCycles) <= UnpredCost;
 }
 
 bool ARMBaseInstrInfo::
 isProfitableToIfCvt(MachineBasicBlock &TMBB,
                     unsigned TCycles, unsigned TExtra,
                     MachineBasicBlock &FMBB,
                     unsigned FCycles, unsigned FExtra,
                     const BranchProbability &Probability) const {
   if (!TCycles || !FCycles)
     return false;
 
   // Attempt to estimate the relative costs of predication versus branching.
-  unsigned TUnpredCost = Probability.getNumerator() * TCycles;
-  TUnpredCost /= Probability.getDenominator();
-
-  uint32_t Comp = Probability.getDenominator() - Probability.getNumerator();
-  unsigned FUnpredCost = Comp * FCycles;
-  FUnpredCost /= Probability.getDenominator();
-
+  unsigned TUnpredCost = Probability.scale(TCycles);
+  unsigned FUnpredCost = Probability.getCompl().scale(FCycles);
   unsigned UnpredCost = TUnpredCost + FUnpredCost;
   UnpredCost += 1; // The branch itself
   UnpredCost += Subtarget.getMispredictionPenalty() / 10;
 
   return (TCycles + FCycles + TExtra + FExtra) <= UnpredCost;
 }
 
 bool