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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:1275
+
+    if (LU.Kind == LSRUse::Address && TTI.LSRWithInstrQueries()) {
+      if (!TTI.isFoldableMemAccessOffset(Fixup.UserInst, Offset))
----------------
jonpa wrote:
> qcolombet wrote:
> > Why would we guard that check with TTI.LSRWithInstrQueries, whereas it was guarded previously.
> The idea here is that ImmCost is not updated with Offset when Target checks it for each fixup in isFoldableMemAccessOffset(). I had found loop regressions  where the NumBaseAdds were the same, but ImmCost were different, and I found that this would be resolved by not updating ImmCost with Offset. I am not sure exactly why -- my guess is that the better formulas (including pre-LSR/input) go first.
> 
Two things regarding this comment:
--- 1. ---

The idea was that if NumBaseAdds is the same, ImmCost is used as a tie breaker.
If I understand correctly you're saying not using this as a tie breaker generates better code. I found that concerning.

Could you dig into that before moving forward?

Assuming you're guess is correct, we should document that fact and make sure the order in the list is not pure luck.

--- 2. ---

This does not answer the question why we should guard this check with LSRWithInstrQueries given it wasn't guarded previously. My concern here is that we change a fairly high weighted piece of the formulae rating and given all other target would return false for LSRWithInstrQueries, I am afraid it will affect their performance across the board.


https://reviews.llvm.org/D35049