Page MenuHomePhabricator

[AttributeList] Change indexes in AttributeList::AttrIndex
AbandonedPublic

Authored by aeubanks on Sep 7 2021, 3:22 PM.

Details

Summary

The indexes used in AttributeList's methods are very confusing. The
index used for the function was (unsigned) -1, return value 0, and the
arguments 1+. When iterating through indexes, we rely on unsigned
wrapping.

This changes the function index to 0, return value index to 1, and
arguments to 2+.

There are two places we have to keep the old indexes. One is the C API,
and the other is reading/writing bitcode for backwards compatibility.

If this breaks any downstream users, inspect calls to
*AttributeAtIndex() to see if they've hardcoded indexes.

Diff Detail

Unit TestsFailed

TimeTest
390 msx64 debian > Clang.CodeGenOpenCL::cl20-device-side-enqueue.cl
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/14.0.0/include -nostdsysteminc /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl -cl-std=CL2.0 -ffake-address-space-map -O0 -emit-llvm -o - -triple "spir-unknown-unknown" | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl --check-prefix=COMMON --check-prefix=B32
620 msx64 windows > Clang.CodeGenOpenCL::cl20-device-side-enqueue.cl
Script: -- : 'RUN: at line 1'; c:\ws\w5\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w5\llvm-project\premerge-checks\build\lib\clang\14.0.0\include -nostdsysteminc C:\ws\w5\llvm-project\premerge-checks\clang\test\CodeGenOpenCL\cl20-device-side-enqueue.cl -cl-std=CL2.0 -ffake-address-space-map -O0 -emit-llvm -o - -triple "spir-unknown-unknown" | c:\ws\w5\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w5\llvm-project\premerge-checks\clang\test\CodeGenOpenCL\cl20-device-side-enqueue.cl --check-prefix=COMMON --check-prefix=B32

Event Timeline

aeubanks created this revision.Sep 7 2021, 3:22 PM
aeubanks published this revision for review.Sep 7 2021, 3:26 PM
aeubanks added reviewers: dexonsmith, rnk.
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2021, 3:26 PM
rnk added a subscriber: tstellar.Sep 8 2021, 2:13 PM

This seems release note worthy. This seems likely to affect every LLVM frontend. +@tstellar for some distributor PoV.

I think one of the reasons I held off on doing this was because I knew we'd have to do the conversion in the C API and the bitcode serialization, and that was two places that needed to know about the index conversion instead of one in the internals.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
791–792

typo s/if/is/

llvm/lib/IR/Core.cpp
2464

Re: the lint check, the static helpers in this file do start with lower case names. I also suggest making the directionality a bit clearer, like mapToLlvmAttributeIndex or something like that. We don't need mapFrom, but I was wondering that to myself.

llvm/unittests/IR/AttributesTest.cpp
175

Hm, this seems like an annoying API break that most frontends will discover only at runtime.

aeubanks updated this revision to Diff 373079.Sep 16 2021, 2:46 PM

address comments

nikic added a reviewer: nikic.Sep 16 2021, 3:05 PM
nikic added a subscriber: nikic.

I'm not convinced that this change is worthwhile. After your recent cleanup, there's not a lot of places working directly with attribute indexes. Especially because we need to keep backwards-compatibility in multiple places, I don't think this change makes things clearer -- especially the fact that "attribute index" in the C API and in the C++ API will now have different meanings is concerning. I can't even say that the new indexes are less arbitrary than the old ones -- the only real advantage they have is that they're directly usable as array offsets.

the main reason I wanted to do this was to avoid the weird unsigned wrapping when doing

for (unsigned i = AL.index_begin(), e = AL.index_end(); i != e; ++i)

but maybe that's not a good enough reason

perhaps instead we could remove the index_begin/index_end methods and provide an iterator via something like indexes

the main reason I wanted to do this was to avoid the weird unsigned wrapping when doing

for (unsigned i = AL.index_begin(), e = AL.index_end(); i != e; ++i)

but maybe that's not a good enough reason

perhaps instead we could remove the index_begin/index_end methods and provide an iterator via something like indexes

https://reviews.llvm.org/D110885

aeubanks abandoned this revision.Fri, Oct 1, 11:05 AM