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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

A few comments/questions, looking only at the SystemZ-specific parts.

I'm now wondering what the difference between isLegalAddressingMode and isFoldableMemAccessOffset is, now that they both take a specific instruction.  Don't these two functions now answer the exact same question?  And if so, shouldn't the implementation then be merged?  The following questions about differences between the two would be moot if we actually had a shared implementation here.

For example, you change isLegalAddressingMode to check for hasNoIndexReg, but not needsD12.  If the function does get an Instruction, and we can see that it won't accept large displacements, shouldn't we then reject the address from isLegalAddressingMode as well?

Also, the various checks for float/vector access in isFoldableMemAccessOffset -- shouldn't they move to hasLessAddressing?  We know that float/vector accesses won't accept large displacements, so shouldn't the function say so?

The comment before hasLessAddressing talks about accesses being converted to vector instructions, but that happens only on z13 or later.  Shouldn't this then be guarded by a Subtarget feature check just like the existing code in isFoldableMemAccessOffset does?

Finally, I'm not really happy about the names ... hasLessAddressing sound a bit strange.   Maybe instead a function called "supportedAddressingMode" that takes an Instruction and returns a description of the addressing mode (long vs. short displacement, index vs. no index), either as an enum or as a pair of booleans,


https://reviews.llvm.org/D35049