[PATCH] D29862: LSR: an alternative way to resolve complex solution

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi,

Thanks for the clarification.

> It should be generally faster.

I was confused by this statement thinking you were trying to improve compile time :).

LGTM with two caveats:

1. Should we set the option to true by default? I believe it would be best to keep it to false and send an email to llvm-dev to ask for benchmarking.
2. The floating point thingy that I mentioned.

I am fine with moving forward with the current patch as long as you commit to look at #2.

Cheers,
Q.



================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:4337
+    float FMinRegNum = LU.Formulae[0].getNumRegs();
+    float FMinARegNum = LU.Formulae[0].getNumRegs();
+    size_t MinIdx = 0;
----------------
evstupac wrote:
> qcolombet wrote:
> > I am not a fan of using float in heuristic as they can introduce subtle difference from one target to other.
> > Could we keep the numerator and denominator as two separate variables and do the comparison accordingly?
> > 
> > We may already have helper class for that. (what is used in BlockFrequency maybe?)
> >I am not a fan of using float in heuristic as they can introduce subtle difference from one target to other.
> >Could we keep the numerator and denominator as two separate variables and do the comparison accordingly?
> 
> Yes. That is doable. However, most likely will be slower.
> I'll take a look what we can do here (maybe using BlockFrequency).
> 
> 
BlockFrequency is probably not the best abstraction but maybe it uses something we can reuse here.


Repository:
  rL LLVM

https://reviews.llvm.org/D29862