[PATCH] D36073: [CGP] Extends the scope of optimizeMemoryInst optimization

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2617
   }
+  bool EqualsIgnoreBase(const ExtAddrMode &O) const {
+    return (ScaledReg == O.ScaledReg) && (BaseGV == O.BaseGV) &&
----------------
mkazantsev wrote:
> I think this can be merged separately as NFC. Also you could re-define the `operator==` avode like `return (BaseReg == O.BaseReg) && EqualsIgnoreBase(O)` to avoid code duplication.
Please take Max's suggestion.  Getting this integrated is going to be a slow process and we should carve out any piece we can.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4617
+      else
+        BB = &MemoryInst->getParent()->getParent()->getEntryBlock();
+      AddrToBase.insert({ { V, BB }, NewAddrMode.BaseReg });
----------------
getParent()->getParent() --> getFunction()


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4630
+    // we find common base later.
+    if (AddrMode.EqualsIgnoreBase(NewAddrMode))
+      // All bases must be of the same type.
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > Better use && instread of increased nesting.
> UPD: Not viable here, sorry, I misread it.
Why is it not viable?


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4645
+    // phi as address is represented by.
+    AddrModeFound = AddrMode.BaseGV || AddrMode.BaseOffs || AddrMode.Scale;
+    if (AddrModeFound)
----------------
This snippet of code confuses me and the comment doesn't help.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4649
+      // this phi as new base.
+      if (!(AddrMode.BaseReg = FindCommonBase(
+                AddrToBase, AddrMode.BaseReg->getType(),
----------------
As I pointed out in the other review (https://reviews.llvm.org/D38133), I think both can be done using the same API as seen by this function.  I generally like the framing of the other patch (track the addrmodes found, then merge later) a bit better than this and it's clearly more general.  I think splitting this patch into two would simply the review greatly and highlight the common work between the reviews.  My suggested split would be:
1) The changes to this function required to track which components differ.  With a trivial implementation of the analysis/transform for when one component differs and the necessary phi/select can be trivially found in the same basic block.
2) A patch which builds on top of that with the rest of the algorithm.

(Note: This is in addition to the select handling factored out from that other review.)


https://reviews.llvm.org/D36073