[PATCH] D40375: Use MemorySSA in LICM to do sinking and hoisting.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1599
+                           SmallPtrSetImpl<MemoryAccess *> &Processed) {
+  if (Processed.count(Acc))
+    return false;
----------------
sanjoy wrote:
> The usual idiom I've seen for this is:
> 
> ```
> if (!Processed.insert(Acc).second)
>   return false;
> ```
> 
Code in question removed.




================
Comment at: lib/Transforms/Scalar/LICM.cpp:1600
+  if (Processed.count(Acc))
+    return false;
+  if (MSSA->isLiveOnEntryDef(Acc))
----------------
sanjoy wrote:
> Should we be returning `true` in this recursive case?  IE if we have a `MemoryPhi` that has one of its inputs as itself and the other input as live-on-entry then shouldn't that be equivalent to just a live-on-entry?
Right, but code in question removed.




================
Comment at: lib/Transforms/Scalar/LICM.cpp:1610
+    Processed.insert(Acc);
+    for (int I = 0; I < NoArgsPhi; I++) {
+      MemoryAccess *Arg = Phi->getIncomingValue(I);
----------------
sanjoy wrote:
> How about a range for on `Phi->operands()`?  Or even better -- `return llvm::all_of(Phi->operands(), <lambda>);`
If using `Phi->operands()` I need to re-add the conversion from Use to MemoryAccess hidden in the macro  in getIncomingValue().
New code would be:
```
return llvm::all_of(Phi->operands(), [MSSA, &Processed](const Use &Arg) {           
      return isVolatileLoadOrLiveOnEntry(
          cast_or_null<MemoryAccess>(Arg.get()), MSSA, Processed);                      
    });
```
Does this look better than existing?

Scratch that, code in question removed.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1617
+    Instruction *AccI = Def->getMemoryInst();
+    if (AccI && isa<LoadInst>(AccI)) // a load is a def if volatile
+      return true;
----------------
sanjoy wrote:
> Is this correct even if defining access for `Def` is a store in the loop?
Code updated, see comment below.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1650
+        // if the Source is a combination of liveOnEntry and
+        // volatile loads (treated as Defs), it's not invalidated.
+        SmallPtrSet<MemoryAccess *, 4> Processed;
----------------
sanjoy wrote:
> This seems fishy -- if `volatile` loads do not invalidate pointers then why are they treated as defs by MSSA?
Well, because they should be treated as defs. Motivation for special casing this was test/Transforms/LICM/volatile-alias.ll.
I updated the code to no longer consider volatile loads acceptable, meaning test in question will fail. And I believe that's the right thing.


https://reviews.llvm.org/D40375