[PATCH] D133192: [LICM] Allow load-only scalar promotion in the presence of aliasing loads

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

This looks like a really nice generalization.  I'd love to take this, but given the compile time cost we need a bit of justification.  In particular, the code size increases on compile-time-tracker are a bit concerning.

Ideas on compile time optimizations inline.



================
Comment at: llvm/lib/Analysis/AliasSetTracker.cpp:254
+        Inst, MemoryLocation(I.getPointer(), I.getSize(), I.getAAInfo()));
+    if (isModAndRefSet(MR))
+      return MR;
----------------
I think you can early return here if mod is set.  The only reason to check for both is if the result mod-noref is useful, and I don't think it is right now. 


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2236
   // We're only interested in must-alias sets that contain a mod.
-  SmallVector<const AliasSet *, 8> Sets;
+  SmallVector<std::pair<const AliasSet *, bool>, 8> Sets;
   for (AliasSet &AS : AST)
----------------
I think you can use a separate set to track the bool property until return.  This might be worthwhile as std::pair<ptr, bool> is pretty space inefficient.  (i.e. use something closer to struct of arrays than array of structs)

Or could we use a tagged pointer here?  That would reduce space wastage to zero.  


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2245
   // Discard any sets for which there is an aliasing non-promotable access.
   foreachMemoryAccess(MSSA, L, [&](Instruction *I) {
     if (AttemptingPromotion.contains(I))
----------------
Since loads no longer cause set pruning, could we maybe visit stores first (as they might) and then only visit readonly instructions afterwards?


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2260
+  for (auto [Set, HasReadsOutsideSet] : Sets) {
     SmallSetVector<Value *, 8> PointerMustAliases;
     for (const auto &ASI : *Set)
----------------
Surely we have a more efficient way to copy a small set vector?


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

https://reviews.llvm.org/D133192