This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Use knowledge retained in llvm.assume (operand bundles)
ClosedPublic

Authored by jdoerfert on Feb 20 2020, 12:15 AM.

Details

Summary

This patch integrates operand bundle llvm.assumes [0] with the
Attributor. Most IRAttributes will now look at uses of the associated
value and if there are llvm.assume operand bundle uses with the right
tag we will check if they are in the must-be-executed-context (around
the context instruction).

This includes a bug fix for KnowledgeRetention.

[0] http://lists.llvm.org/pipermail/llvm-dev/2019-December/137632.html

Diff Detail

Event Timeline

jdoerfert created this revision.Feb 20 2020, 12:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2020, 12:15 AM

Rebase and more tests

Simplify code

uenoku added inline comments.Feb 21 2020, 3:12 AM
llvm/lib/Transforms/IPO/Attributor.cpp
680

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?

744–749

Can't we share AssumeAttrMap with other AAs whose associated values are the same?

jdoerfert marked 2 inline comments as done.Feb 22 2020, 10:50 PM
jdoerfert added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
680

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.

744–749

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.

Tyker added inline comments.Feb 23 2020, 9:41 AM
llvm/lib/Transforms/IPO/Attributor.cpp
744–749

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.

uenoku added inline comments.Feb 24 2020, 3:13 AM
llvm/lib/Transforms/IPO/Attributor.cpp
680

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;
}
744–749

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.

Use (an extended) map interface to retain (and cache) knowledge

uenoku accepted this revision.Mar 14 2020, 1:32 AM

LGTM

This revision is now accepted and ready to land.Mar 14 2020, 1:32 AM
This revision was automatically updated to reflect the committed changes.