[PATCH] D96399: [X86][CodeGenPrepare] Try to reuse IV's incremented value instead of adding the offset

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/required comments.



================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:3890
+  //    assignment.
+  if (AddrMode.BaseOffs) {
+    if (auto IVStep = GetConstantStep(ScaleReg)) {
----------------
I'm not really convinced of your second point above (e.g. the generic offset one).  In particular, it's not clear to me that an arbitrary offset is *always* better than an overlapped live interval.  

I'm also not convinced you're wrong.

I'm not going to require you stage this, but if you need to revert for any reason I'll strongly suggest staging first the case where the existing base offset cancels, and then doing the general offset case in a second patch.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:3897
+        TestAddrMode.ScaledReg = IVInc;
+        TestAddrMode.BaseOffs -= Step.getLimitedValue() * AddrMode.Scale;
+        // If this addressing mode is legal, commit it..
----------------
As written, there's a potential overflow here for SINT_MAX and Scale = 8.  Please use APInt.smul_ov here.  


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:3900
+        if (TLI.isLegalAddressingMode(DL, TestAddrMode, AccessTy, AddrSpace)) {
+          AddrModeInsts.push_back(cast<Instruction>(ScaleReg));
+          AddrMode = TestAddrMode;
----------------
If I'm reading the code correctly, you want this to be adding IVInc to the instruction list, not the phi node.  


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96399/new/

https://reviews.llvm.org/D96399