[PATCH] D78014: [AssumeBundles] Prevent generation of some redundant assumes

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D78014#1983030 <https://reviews.llvm.org/D78014#1983030>, @Tyker wrote:

> i ran the compile-time benchmark, the previous version of the patch had some significant slowdown but disabling tryToPreserveWithoutAddingAssume when no AssumptionCache is available brought the difference within noise levels.
>  the measurement was between current master and master + all the AssumeBundles patch not yet committed with knowledge retention enabled.


I think we can go ahead. The pass might reuse this logic. I'll give it another look later though.

> you are probably right that doing this on the flight isn't enought and we still need a pass to prune and regroup assume bundles.
>  this seems like it should be run quite often since many passes can affect assume bundles.
>  any thought ?

I don't think it will be too bad. Given this patch, trivial assumes are not placed so we get some on-the-fly pruning.
We need to run tests but I can imagine to run the new pass after things like the inliner and Attributor. The first creates opportunities
to merge/prune assumes in the new function, the second manifests information in other places reducing the need for assumes. At the end
of the day the number of times we run it might not matter if it is fast. The OpenMPOpt pass looks for runtime calls and where they are to
determine if any work should be done. That is, the pass is a no-op if no `llvm.assume` is present. The pass can ignore all functions without
`llvm.assume`. All in all, it scales with the assumes and I hope the queries we run are not really expensive (e.g., DomTree queries).
I'm not sure if we want this as part of the Attributor or not.



================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:77
   Instruction *InsertBeforeInstruction = nullptr;
-
+  AssumptionCache* AC;
+  DominatorTree* DT = nullptr;
----------------
Tyker wrote:
> jdoerfert wrote:
> > It should be nullptr or better initialized by the constructor. Same for the other members. There should be a constructor to initialize all of this and nullptrs otherwise.
> personally i prefer not having a constructor (or function) taking too many argument as i find it harder to read and write.
> but AC should definitely be default initialized to nullptr.
This looks good now. I hope this is OK for you as well.

FWIW, I want to avoid the pattern that you create via constructor, then initialize some members later on via a second scheme. That is complicated and error prone, e.g., it is easy to forget the second step.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78014