[PATCH] D89264: [LICM] Make promotion faster

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea accepted this revision.
asbirlea added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:461
+        foreachMemoryAccess(MSSA, L, [&](Instruction *I) {
+          MaybePromotable.push_back(I);
+        });
----------------
nikic wrote:
> asbirlea wrote:
> > Nit: It seems the conditions in `IsPotentiallyPromotable` would be useful as an early filter to prevent filling up the `MaybePromotable` only to clean it up shortly after.
> The idea here is that some accesses may not be promotable initially, but may become promotable after others have been promoted. Each iteration removes all potentially promotable accesses from `MaybePromotable`, leaving the rest for the next iteration (if any promotion happened). For this reason we can't do early filtering here. (See also the comment on the loop below.)
I misread the condition on which accesses are kept in `MaybePromotable`. This is fine.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2274
+  });
+
+  // We're only interested in must-alias sets that contain a mod.
----------------
nikic wrote:
> asbirlea wrote:
> > A quick exit check for promotion is whether the AST is saturated.
> > Probably need to add the API for that inside the AliasSetTracker.
> > ```
> > bool isSaturated(){
> >     return AliasAnyAS != nullptr;
> > }
> > ```
> > 
> > ```
> > if (AST.isSaturated())
> >     return {}; // Nothing to promote...
> If I'm understanding how this works correctly, won't the "saturated" case be handled automatically? If the AST is saturated, then there will only be a single may-alias set, so the loop below will only iterate once and return afterwards.
Yes, you're right, this is good.


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

https://reviews.llvm.org/D89264