[PATCH] D32819: [IR] Switch AttributeList to use an array for O(1) access

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked an inline comment as done.
rnk added a comment.

In https://reviews.llvm.org/D32819#749163, @rnk wrote:

> One question remains: do you think I should completely encapsulate this indexing change by adding 1 inside AttributeList::getAttributes, or is it better to update all the callers as I did in this change? The problem with this change is that it will break out-of-tree backends and frontends that use the APIs with one-based indexing. It won't even be a compile failure because the APIs still take `unsigned` integers. That's a bit rude without some more hand waving on llvm-dev and release notes.


@javed.absar suggested that this was worth it, so I tried it, but I don't like the result. It means this doesn't work anymore:

  // Do lookups for all attribute groups.
  for (unsigned i = 0, e = PAL.getNumAttrSets(); i != e; ++i) {
    AttributeSet AS = PAL.getAttributes(i);
    if (!AS.hasAttributes())
      continue;
    IndexAndAttrSet Pair = {i, AS};
    unsigned &Entry = AttributeGroupMap[Pair];
    if (Entry == 0) {
      AttributeGroups.push_back(Pair);
      Entry = AttributeGroups.size();
    }
  }

We would need to expose something like index_begin() / index_end() / indices() to make it easy to iterate over AttributeList indices. If we need to have a loop over indices, that loop should start at zero. Anything else is surprising.



================
Comment at: llvm/include/llvm/IR/Attributes.h:287
 public:
   enum AttrIndex : unsigned {
+    FunctionIndex = 0,
----------------
rnk wrote:
> javed.absar wrote:
> > Yes, make the code much neater.  Unfortunately, because of legacy stuff, as you mention, adjustments are needed in lot of places. I think encapsulating the index change inside AttributeList might be neater (less pervasive change).
> OK, I'll go ahead and split the changes.
I tried this and I don't like the result. I'll explain more in a top-level comment.


https://reviews.llvm.org/D32819