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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2617
   }
+  bool EqualsIgnoreBase(const ExtAddrMode &O) const {
+    return (ScaledReg == O.ScaledReg) && (BaseGV == O.BaseGV) &&
----------------
reames wrote:
> 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.
We agreed that john.brown will implement it in a separate patch. So I 'm waiting him for this update.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4630
+    // we find common base later.
+    if (AddrMode.EqualsIgnoreBase(NewAddrMode))
+      // All bases must be of the same type.
----------------
reames wrote:
> mkazantsev wrote:
> > mkazantsev wrote:
> > > Better use && instread of increased nesting.
> > UPD: Not viable here, sorry, I misread it.
> Why is it not viable?
Because there is an additional code with we should execute if first condition is true while the second one is false.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4645
+    // phi as address is represented by.
+    AddrModeFound = AddrMode.BaseGV || AddrMode.BaseOffs || AddrMode.Scale;
+    if (AddrModeFound)
----------------
reames wrote:
> This snippet of code confuses me and the comment doesn't help.
I meant that if AddrModes contains only base (no index, no offset), no need to do a complex analysis because we end up by the same Phi node as we start from. 


https://reviews.llvm.org/D36073