[PATCH] D35933: Eliminate TargetTransformInfo::isFoldableMemAccess()

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

The SystemZ part looks correct to me, see the inline comment for a style/readability suggestion.

I'm not sure about the common code changes -- what does simply calling isLegalAddressingMode instead of isFoldableMemAccessOffset do to targets that have not yet added support to do instruction-specific checks to the former?  I'd assume you'd see performance regressions there.

Maybe this transition should be done on a target-by-target basis.



================
Comment at: lib/Target/SystemZ/SystemZISelLowering.cpp:699
 
-  if (I != nullptr &&
-      !supportedAddressingMode(I, Subtarget.hasVector()).IndexReg)
-    // No indexing allowed.
-    return AM.Scale == 0;
-  else
-    // Indexing is OK but no scale factor can be applied.
-    return AM.Scale == 0 || AM.Scale == 1;
-}
-
-// TODO: Should we check for isInt<20> also?
-bool SystemZTargetLowering::isFoldableMemAccessOffset(Instruction *I,
-                                                      int64_t Offset) const {
-  if (!supportedAddressingMode(I, Subtarget.hasVector()).LongDisplacement)
-    return (isUInt<12>(Offset));
+  if (I != nullptr) {
+    AddressingMode SupportedAM = supportedAddressingMode(I, Subtarget.hasVector());
----------------
Hmm, this pulls the conditions apart again ...   Maybe readability would be improved by just always having a SupportedAM variable, initialized at the top like:

```
 AddressingMode SupportedAM(true, true);
 if (I != nullptr)
    SupportedAM = supportedAddressingMode(I, Subtarget.hasVector());

```
and then just use it throughout the function.


https://reviews.llvm.org/D35933