[PATCH] D78804: [LICM] Precompute memory writers for AST aliasing analysis

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

Looks good overall, just a few nits inline.



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:143
                   BasicBlock *Dest, ICFLoopSafetyInfo *SafetyInfo,
+                  Optional<DenseSet<Instruction *> > LoopMemWrites,
                   MemorySSAUpdater *MSSAU, ScalarEvolution *SE,
----------------
` Optional<DenseSet<Instruction *> > ` clang-format adds that space between `> >`?
It's consistent throughout, so I guess so...it just looks odd to me :-).


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:313
     LLVM_DEBUG(dbgs() << "LICM: Using Alias Set Tracker.\n");
     CurAST = collectAliasInfoForLoop(L, LI, AA);
   } else {
----------------
Does it make sense to create the DenseSet here and pass it to the sinkRegion and hoistRegion below, instead of creating in each?

It may save some time to only create once if the set it properly updated after each of those calls, but then it's also expanding the signature of sinkRegion and hoistRegion.
An option may be to put the set inside a data structure, such as the `SinkAndHoistLICMFlags`?


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:811
+          eraseInstruction(I, *SafetyInfo, CurAST, LoopMemWrites, MSSAU);
+        }
         Changed = true;
----------------
NIt: no braces


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1563
+    LoopMemWrites->erase(&I);
+  }
+
----------------
Nit: no braces.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1645
   }
+
   return Changed;
----------------
Nit: no new line needed.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1666
+    LoopMemWrites->erase(&I);
+  }
+
----------------
Nit: no braces.


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

https://reviews.llvm.org/D78804