[PATCH] D74888: [Attributor] Use knowledge retained in llvm.assume (operand bundles)

[Johannes Doerfert via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:681
 bool IRPosition::hasAttr(ArrayRef<Attribute::AttrKind> AKs,
-                         bool IgnoreSubsumingPositions) const {
+                         bool IgnoreSubsumingPositions, Attributor *A) const {
   SmallVector<Attribute, 4> Attrs;
----------------
uenoku wrote:
> I think using `nullptr` as a flag to indicate subsuming positions seems a bit difficult to read. 
> Could you simply add some boolean flag and replace the argument with a reference?
I'm not sure I understand. If `A` is null, you don't get any assume handling. If `A` is not null, you get assume handling but only for the current position. not any subsuming one.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:748-753
+  for (const Use &U : AssociatedValue.uses())
+    if (IntrinsicInst *II = dyn_cast_or_null<IntrinsicInst>(U.getUser()))
+      if (II->getIntrinsicID() == Intrinsic::assume && !II->isArgOperand(&U))
+        if (hasAttributeInAssume(*II, &AssociatedValue, AttrName, ArgValPtr))
+          if (!ArgValPtr || ArgVal > 0)
+            AssumeAttrMap[II] = Attribute::get(Ctx, AK, ArgVal);
----------------
uenoku wrote:
> Can't we share `AssumeAttrMap` with other AAs whose associated values are the same?
Yes, that is the plan. In KnowledgeRetention.{h/cpp} there is a TODO that says we should provide an API to get all information from an `llvm.assume` in a map. Once that is in place, the Attributor should be the one doing the querying (once) and then caching the information. Does that make sense?

I hoped @Tyker will look into that API at some point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74888