Page MenuHomePhabricator

[IR] Make AttributeSetNode public, avoid temporary AttributeList copies
ClosedPublic

Authored by rnk on Mar 21 2017, 11:21 AM.

Details

Summary

AttributeList::get(Fn|Ret|Param)Attributes no longer creates a temporary
AttributeList just to hide the AttributeSetNode type.

I've also added a factory method to create AttributeLists from a
parallel array of AttributeSetNodes. This greatly simplified the
construction of AttributeLists during IPO transforms, in my opinion.
Previously we would test if a particular index had attributes, and
conditionally add a temporary attribute list to a vector. Now the
attribute set vector is parallel to the argument vector already that
these passes already construct.

My long term vision is to wrap AttributeSetNode* inside an AttributeSet
type that holds the enum attributes, but that will come in a follow up
change.

I haven't done any performance measurements for this change because
profiling hasn't shown that any of the affected code is hot.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Mar 21 2017, 11:21 AM
rnk updated this revision to Diff 92531.Mar 21 2017, 1:00 PM
  • Fix bug in addAttribute implementation
rnk updated this revision to Diff 94574.Apr 7 2017, 4:10 PM
  • rebase
pete accepted this revision.Apr 10 2017, 1:10 PM

Sorry for forgetting to review this.

LGTM with just these comments (but only comments so feel free to commit).

My only worry is that I view AttributeSetNode as an internal detail of the Attribute implementation. I think this is an incremental patch which eventually gets us to a cleaner set of APIs, so I'm fine with it for now, but I think the eventual goal shouldn't have AttributeSetNode be visible any more.

Note, when i looked at this a while ago, i created something I called AttributeRef which I guess was effectively the same as what you have here. But it still suffered from similar issues in terms of leaking implementation details. Its also possible that moving Attributes to the parameters just fixes most of this anyway.

This revision is now accepted and ready to land.Apr 10 2017, 1:10 PM
rnk added a comment.Apr 10 2017, 1:17 PM
In D31198#722892, @pete wrote:

Sorry for forgetting to review this.

NP, I got distracted by PDB stuff for a bit anyway.

LGTM with just these comments (but only comments so feel free to commit).

My only worry is that I view AttributeSetNode as an internal detail of the Attribute implementation. I think this is an incremental patch which eventually gets us to a cleaner set of APIs, so I'm fine with it for now, but I think the eventual goal shouldn't have AttributeSetNode be visible any more.

Note, when i looked at this a while ago, i created something I called AttributeRef which I guess was effectively the same as what you have here. But it still suffered from similar issues in terms of leaking implementation details. Its also possible that moving Attributes to the parameters just fixes most of this anyway.

I agree. I plan to wrap it back up in a new type called AttributeSet that will hold the enum attributes locally, so we can do much faster hasAttr tests.

This revision was automatically updated to reflect the committed changes.