[PATCH] D35049: LSR tunings for SystemZ, with some minor common code changes

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

I see why `second batch` depends on this patch. I wonder if it should though.
Hence, do we rely guard that check with LSRWithInstrQueries?



================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:1657
+    return true;
+  }
+
----------------
jonpa wrote:
> qcolombet wrote:
> > I feel that this code does not belong here.
> > 
> > Indeed, we have quite a few isAMCompletelyFolded overloaded functions, and I believe not all invocations would go through that specific instance.
> > Instead, I would have expected this to happen to the lower most version of the isAMCompletelyFolded version. The one that calls isLegalAddressingMode.
> The reason that I put it here, is because this is where LU is available. The check can't be done without LU (which has the Fixups), so if it's not placed here, the argument lists of the other versions must be changed, as well as the call sites of it and isLegalUse() (and possibly more?) to make the Fixups available. Is this what you have in mind, and if so should LU replace the other LU-arguments like MinOffset etc?
> 
The thing that I don't like is that what we're basically copying what's inside the bottom most version of isAMCompletelyFolded in the Address case. If possible I would have liked we call that code.

Could we keep that loop here but call the bottom most isAMCompletelyFolded with an additional Instr parameter?  


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:1275
+
+    if (LU.Kind == LSRUse::Address && TTI.LSRWithInstrQueries()) {
+      if (!TTI.isFoldableMemAccessOffset(Fixup.UserInst, Offset))
----------------
Why would we guard that check with TTI.LSRWithInstrQueries, whereas it was guarded previously.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:1279
+    } else
+      C.ImmCost += APInt(64, Offset, true).getMinSignedBits();
   }
----------------
This slightly changes how/when we accumulate ImmCost.
Is that intentional?


https://reviews.llvm.org/D35049