[PATCH] D46193: [LSR] Skip LSR if the cost of input is cheaper than LSR's solution

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added a comment.

> Compile it with clang -target hexagon -O3. The code you added eventually punts (FormInputLSRUseAndFormula returns false), and LSR proceeds to do its thing. I did some analysis, and the problem is with  %add.ptr = getelementptr inbounds i32, i32* %2, i32 %Col.0. This is not an "address use", since it goes into another GEP. It exists in the original source, but LSR never looks at it. Your code does and that makes it exit early. Maybe you should restrict the uses you look at to the same ones that LSR starts with?

We should consider costs of all SCEVable instructions between phi and leaf IV users. When creating the initial formula, those SCEVable instructions in the middle must be folded as a part of formula, but to find the input cost without LSR transformation we should count costs of all those instructions.



================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:3326
+LSRUse &LSRInstance::GetOrCreateLSRUse(
+    Instruction *UserInst, Value *IVOp, const SCEV *S, LSRUse::KindType Kind,
+    MemAccessTy &AccessTy, PostIncLoopSet &TmpPostIncLoops, UseMapTy &LsrUseMap,
----------------
rehana wrote:
> The "S" parameter is missing the "&" and it must be added. This function calls getUse() with "S", and getUse modifies the parameter. Without the "&", the modification by getUse will not be seen by the caller of GetOrCreateLSRUse.
Thanks for catching !


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:5679
+      IsInputCostCheaperThanSolutionCost(SolutionCost)) {
+    DEBUG(dbgs() << "Skip using LSR's solution.\n");
+    return;
----------------
sebpop wrote:
> Do you have some statistics on how many times this currently happens on a benchmark of your choice?
> 
For now I want to be conservative on skipping LSR even when the input cost is shown to be cheap, so I applied some weight on the input cost before comparing with the selected solution. Because of this weight, it doesn't seem to happen widely. In my test for spec2000, it impact only on 6 loops.


================
Comment at: test/Transforms/LoopStrengthReduce/AArch64/skip-lsr-solution.ll:1
+; RUN: llc <  %s  -mtriple=aarch64 -lsr-insns-cost=true -debug-only=loop-reduce  2>&1 | FileCheck %s
+
----------------
kparzysz wrote:
> sebpop wrote:
> > kparzysz wrote:
> > > This needs "REQUIRES: asserts".
> > I don't see any CHECK statement depending on -debug-only, so instead of requiring asserts, let's just remove that flag.
> > 
> > Also please remove the other flag: -lsr-insns-cost=true as I see that its default value is true:
> > 
> >   "lsr-insns-cost", cl::Hidden, cl::init(true),
> > 
> It's there, the first CHECK line: `CHECK: Skip using LSR's solution`.
In order to force to use #instruction in the cost model for this test, we need to have -lsr-insns-cost=true specifically in the command-line because the occurrence of lsr-insns-cost is checked in Cost::isLess().


https://reviews.llvm.org/D46193