[llvm-commits] [llvm] r147286 - in /llvm/trunk: lib/Transforms/Utils/SimplifyCFG.cpp test/Transforms/SimplifyCFG/preserve-branchweights.ll

[Jakob Stoklund Olesen]

On Dec 26, 2011, at 8:31 PM, Nick Lewycky wrote:

Hi Nick, sorry for the late comments. I missed this the first time.

> +      APInt GCD = APIntOps::GreatestCommonDivisor(ProbTrue, ProbFalse);
> +      ProbTrue = ProbTrue.udiv(GCD);
> +      ProbFalse = ProbFalse.udiv(GCD);

This causes pretty weird behavior if you think about it.
Future weight computations are more likely to overflow if weights are co-prime?

> +
> +      if (Overflow1 || Overflow2 || Overflow3 || Overflow4 || Overflow5 ||
> +          Overflow6) {
> +        DEBUG(dbgs() << "Overflow recomputing branch weight on: " << *PBI
> +                     << "when merging with: " << *BI);
> +        PBI->setMetadata(LLVMContext::MD_prof, NULL);

Chicken. ;-)

Please deal with overflow correctly, and scale the new weights to fit in 32 bits. Then you don't need the GCD stuff either.

Branch weights could be actual execution counts, and we don't require them to be scaled down to small integers. The full 32-bit range should be well supported.

I was sure Jakub had written code just like this, but I can't find it now.

/jakob