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

[Hideto Ueno via Phabricator via llvm-commits]

uenoku 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;
----------------
jdoerfert wrote:
> 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.
I meant that the following implementation is more readable for me. Either way is fine anyway.
```
bool IRPosition::hasAttr(ArrayRef<Attribute::AttrKind> AKs,
                         bool IgnoreSubsumingPositions, Attributor &A) const {
  ...
  bool IsSubsumingPosition = false;
  for (const IRPosition &EquivIRP : SubsumingPositionIterator(*this)) {
    for (Attribute::AttrKind AK : AKs) {
      ...
      if (!IsSubsumingPosition && EquivIRP.getAttrsFromAssumes(AK, Attrs, A))
        return true;
    }
    ...
    IsSubsumingPosition = true;
  }
  return false;
}

```


================
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);
----------------
Tyker wrote:
> jdoerfert wrote:
> > 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.
> I added a revision (D75020) with what i thought about when talking about the map API. I hope it fits with what you had in mind.
> 
> but from looking at the code it seems like what could be the most usefull is a way to get from a use in the operand bundles to the knowledge associated with it quickly. and it can be done much quicker than hasAttributeInAssume and may be generally useful.
> 
> also sorry for the bug in hasAttributeInAssume.
> 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?
Ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74888