[PATCH] D126043: [LSR] Drop LSR solution if it is less profitable than baseline

[Yueh-Ting (eop) Chen via Phabricator via llvm-commits]

eopXD added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:3372
+    F.initialMatch(S, L, SE);
+    if (!BaselineCost.isLoser())
+      BaselineCost.RateFormula(F, Regs, VisitedRegs, LU);
----------------
eopXD wrote:
> Meinersbur wrote:
> > How can the baseline immediately become the loser?
> I have encounter cases that the `BaselineCost` is updated to loser when compiling spec2k6 benchmarks. This if-statement is used for us to prevent assertion errors as `RateFormula` assumes the cost to not be a loser when called.
> 
> I have a patch (D125670) that is pretty simple to free us from using this if-statement? I think its pretty simple and we can land it first?
D125670 landed. Marking this thread as complete.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:3373
+    if (!BaselineCost.isLoser())
+      BaselineCost.RateFormula(F, Regs, VisitedRegs, LU);
+
----------------
eopXD wrote:
> Meinersbur wrote:
> > Since SCEV subexpressions can be reused by multiple instructions, doesn't this mean that this is overestimating the baseline cost?
> Yes you are right, we should prevent repeating RateFormula hen multiple `IVStrideUse` matches the same `LU`. Let me update my code.
Created `DenseSet<int> VisitedLSRUse` to take account for SCEV that is already calculated.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5159
+
+  if (!SolutionCost.isLess(const_cast<Cost &>(BaselineCost))) {
+    LLVM_DEBUG(dbgs() << "\n"
----------------
eopXD wrote:
> Meinersbur wrote:
> > Instead of the const cast, would it be possible to make `Cost::isLess` accept a const object?
> Created D126350.
D126350 landed, marking this thread as complete.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5160
+  if (!SolutionCost.isLess(const_cast<Cost &>(BaselineCost))) {
+    LLVM_DEBUG(dbgs() << "\n"
+                         "The baseline solution requires ";
----------------
Meinersbur wrote:
> Starting with newline?
Removed leading newline.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126043/new/

https://reviews.llvm.org/D126043