[PATCH] D35741: Add MemorySSA alternative to AliasSetTracker in LICM.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

I have added some generic style comments.

As discussed in person, I think we should split out the `MemorySSAAliasSets` class, and add some targeted unit tests.

Please also add a comment describing the algorithm at a high level.



================
Comment at: include/llvm/Analysis/MemorySSAAliasSets.h:10
+// This a a helper class that creates alias sets using MemorySSA, each
+// set being marked with properties that enablei/disable promotion.
+// It is used for promotion in the Loop Invariant Code Motion Pass.
----------------
s/enablei/enable/


================
Comment at: include/llvm/Analysis/MemorySSAAliasSets.h:49
+    // witout a MemoryLocation.
+    bool CanNeverPromote=false;
+    bool LoopInvariantAccess=false;
----------------
The whitespace is a bit off here (clang-format?).


================
Comment at: include/llvm/Analysis/MemorySSAAliasSets.h:98
+
+  void addToAliasSets(const MemoryUse *Use, SmallPtrSetImpl<const MemoryAccess*> &Processed) {
+    Processed.insert(Use);
----------------
You probably should split this out into header and a .cpp -- that way telling the interface apart from the implementation will become easier.


================
Comment at: include/llvm/Support/Debug.h:102
+///
+extern bool UseLICMwMSSA;
+
----------------
I'd prefer to spell this out as UseLICMWithMSSA.


================
Comment at: include/llvm/Transforms/Scalar/LoopPassManager.h:350
     PA.preserve<ScalarEvolutionAnalysis>();
+    if (EnableMSSALoopDep) {
+      PA.preserve<MemorySSAAnalysis>();
----------------
LLVM style is to avoid braces around single line statements like this.


================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:10
+// This a a helper class that creates alias sets using MemorySSA, each
+// set being marked with properties that enablei/disable promotion.
+// It is used for promotion in the Loop Invariant Code Motion Pass.
----------------
s/enablei/enable/


================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:28
+
+using namespace llvm;
+
----------------
Opening namespaces in headers is against LLVM style.


================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:34
+  MemorySSA *MSSA;
+  MemorySSAUpdater *MSSAUpdater;
+
----------------
I'd prefer having these under accessors.


================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:43
+
+  class SingleMustAliasVec {
+    MemoryLocation Loc;
----------------
I'm not sure about the name -- how about `MustAliasSet`?  The "single" part is implied by the fact that the name is singular, and this is semantically a "set" and not a "vec".


================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:54
+    bool HasAGoodStore = false;
+    SingleMustAliasVec(MemoryLocation loc) : Loc(loc), CanNeverPromote(false) {
+      Vec.insert(const_cast<Value *>(Loc.Ptr));
----------------
You probably don't need the `CanNeverPromote(false)` bit.


================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:58
+    void addMemoryLocation(Optional<MemoryLocation> MemLoc) {
+      if (MemLoc == None)
+        return;
----------------
This will be a line shorter without an early return.


================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:86
+  unsigned MemorySSATraversalPenalty = 0;
+  unsigned CAP_NO_ALIAS_SETS = 30;
+  unsigned CAP_MSSA_TRAVERSALS = 30;
----------------
These need to be `cl::opt` s

You should also split this header into a .cpp file (and put the cl::opt s in  that .cpp file).


================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:90
+  void dump() {
+    for (SingleMustAliasVec *Set : AllSets)
+      Set->dump();
----------------
You should use `LLVM_ENABLE_DUMP` and `LLVM_DUMP_METHOD` for this.  See the definition and declaration of e.g. `DominanceFrontierWrapperPass::dump()`


================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:96
+    if(!L)
+      return true;
+    return L->isLoopInvariant(V);
----------------
Can be a ternary.  Same for loopContains


================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:112
+                    MemoryLocation &MemLocJ) {
+    if (MemLocI == None) {
+      ModRefInfo ModRef = AA->getModRefInfo(InstrI, MemLocJ);
----------------
I'd structure this as:

```
if (MemLocI)
  return AA->alias(MemLocI.getValue(), MemLocJ);
// Rest of the code
```



================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:247
+    // before their children in the worklist and process the worklist in order.
+    SmallVector<DomTreeNode *, 16> Worklist = collectChildrenInLoop(N, L);
+
----------------
In `loopContains` you've handled the case where `L` is `nullptr`, but you've not done the same here.


================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:253
+      if (Accesses)
+        for (auto &Acc : make_range(Accesses->begin(), Accesses->end())) {
+          if (AllSets.size() > CAP_NO_ALIAS_SETS ||
----------------
Can this be `for (auto &Acc : *Accesses)`?


================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:260
+          // Process uses
+          const MemoryUse *Use = dyn_cast_or_null<MemoryUse>(&Acc);
+          if (Use)
----------------
You can do:

```
if (const auto *Use = dyn_cast_or_null<MemoryUse>(&Acc))
  addToAliasSets(Use);
```

also, the second `if` can be an `else if`.


================
Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:284
+
+  void replace(Value *LoadI, Value *Replacement) {
+    for (auto OneAliasSet : AllSets) {
----------------
Can you please give this a more obvious name, like `replaceMemoryLocation`?

Also, the name `LoadI` is misleading -- it isn't the load instruction, but it is the memory location, right?


https://reviews.llvm.org/D35741