[PATCH] D127392: [AggressiveInstCombine] Combine consecutive loads which are being merged to form a wider load.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:508
+      Load1Ptr = BC->getOperand(0);
+  }
+
----------------
This whole code can be replaced with `stripAndAccumulateConstantOffsets()`.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:538
+  if (!FindAvailableLoadedValue(LI1, LI2->getParent(), BBIt, MaxScan, nullptr,
+                                &IsLoadCSE))
+    return false;
----------------
FindAvailableLoadedValue() is not the correct way to check for clobbers. In particular, it will return an "available value" for a direct clobber (store to the same address).

What you want it to loop over the instructions and call getModRefInfo() on AliasAnalysis, together with a small limit (e.g. 16) when you will abort the walk and bail out of the transform.


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/AArch64/or-load.ll:203
+; ALL-NEXT:    [[L3:%.*]] = load i8, ptr [[P2]], align 1
+; ALL-NEXT:    store i8 10, ptr [[PSTR:%.*]], align 1
+; ALL-NEXT:    [[L4:%.*]] = load i8, ptr [[P3]], align 1
----------------
Try a variant storing to `%p3` rather than `%pstr` here. I believe your current implementation will incorrectly accept this.


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

https://reviews.llvm.org/D127392