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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand accepted this revision.
uweigand added a comment.

> I agree this isn't perfect. I also noticed this before, and thought this could be possibly refactored but however also noticed the many different overloaded versions of isLegalAddressingMode() and thought that it could wait as a next step, perhaps. RateFormula() currently only checks the fixup instructions for the offsets, so I am not sure this is a trivial change.

Right, changing the whole common code interface would be a larger effort.  I was primarily concerned about the //SystemZ implementation// of those routines -- they should be doing the same thing, ideally by sharing code.  However, with this latest version of the patch, they actually already do share most of the code, so my concern is mostly resolved.  If we can go on and unify the offset checks (i.e. have the same 12- or 20-bit checks in both routines), then we should be completely OK.

I understand this change might lead to changes in generated code, which might have other performance impacts.  So I'd be fine with checking in this patch, and then following up with another patch to fix the offset checks (after doing another round of benchmarking).

> Changed it and agree this is more clear by returning both values as a pair. Does it look acceptable now?

Yes, this does indeed look much better now.

The SystemZ changes overall LGTM now, just some minor in-line comments.



================
Comment at: lib/Target/SystemZ/SystemZISelLowering.cpp:623
+
+// TODO: This method should also check for the displacemnt when *I is
+// passed. It may also be possible to merge with isFoldableMemAccessOffset()
----------------
Typo: displacement


================
Comment at: lib/Target/SystemZ/SystemZISelLowering.cpp:643
   // Indexing is OK but no scale factor can be applied.
   return AM.Scale == 0 || AM.Scale == 1;
 }
----------------
Just as a minor readability enhancement, I'd move the supportedAddressingMode check down here, and write the whole thing like this:

```
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;
```


================
Comment at: lib/Target/SystemZ/SystemZISelLowering.cpp:651
 
   return true;
 }
----------------
Hmm.  I know the code didn't before either, but shouldn't we check whether the Offset fits into 20 bits here?  Maybe at least add a TODO if we don't want to change it right now.


https://reviews.llvm.org/D35049