[PATCH] D89264: [LICM] Make promotion faster

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D89264#2573058 <https://reviews.llvm.org/D89264#2573058>, @asbirlea wrote:

> Runtime impact looks reasonable now. What's the compile-time impact for the patch now?

The link in the summary is already an updated run (https://llvm-compile-time-tracker.com/compare.php?from=f197cf2126be3b224cadfe8b1cde9c05f638a0ea&to=7f1e24e26d198e1d80d32c87c9fd1f5cf3cc8c5f&stat=instructions), so this is now ~1.8% geomean improvement at `O3`, with 5.5% on lencod in particular.



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:461
+        foreachMemoryAccess(MSSA, L, [&](Instruction *I) {
+          MaybePromotable.push_back(I);
+        });
----------------
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.)


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2274
+  });
+
+  // We're only interested in must-alias sets that contain a mod.
----------------
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.


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

https://reviews.llvm.org/D89264