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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In https://reviews.llvm.org/D32819#749907, @tobiasvk wrote:

> 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.
>
>
> I'm very concerned about breaking out-of-tree code in a way that's not detectable at compile time. We have a whole bunch of out-of-tree passes that will be affected by this (...I'll fix them since I know about it) but others may not be watching the mailing lists as closely. Of course we make no guarantees about C++ API stability, but I feel we shouldn't cause unnecessary pain and breakage if it can be avoided.


Having just debugged a lot of "index out of bounds" type runtime failures, I generally agree, I don't want to put others through it. :)

> How about renaming the APIs that take or return arg indices? That'll make it abundantly clear that their behavior has changed (and will also easily flag up all the places in upstream code that need to be changed).

I don't want to use bad names (`hasAttributeEx`?) just for backwards compatibility. One idea I had was to make a better API, one that has no attribute list indices at all, but there are a handful of places in LLVM that abstract over argument and return attributes with attribute list indices, and I couldn't find a way to replace them. Maybe rename the old APIs to hasRetOrArgAttr or something painfully ugly like that to discourage people from using them? We'd have (has|get|add|remove)ParamAttr, *RetAttr, *FnAttr to make it easier to migrate code to do the right thing.

Anyway, I'll upload a patch that retains compatibility. I want to get the performance win before dealing with that.


https://reviews.llvm.org/D32819