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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev marked 2 inline comments as done.
mkazantsev added inline comments.


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


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:313
     LLVM_DEBUG(dbgs() << "LICM: Using Alias Set Tracker.\n");
     CurAST = collectAliasInfoForLoop(L, LI, AA);
   } else {
----------------
asbirlea wrote:
> 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`?
I also was considering it and didn't do it because of impact on sink/hoistRegion API. I will take another look, maybe we can indeed encapsulate it somewhere.


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

https://reviews.llvm.org/D78804