Page MenuHomePhabricator

[IR] Use a proper type for AttributeList indexing
Changes PlannedPublic

Authored by gchatelet on Jul 6 2021, 7:59 AM.

Details

Summary

This is a follow up patch on https://reviews.llvm.org/D32811.
It avoids leaking information about the encoding of the index for
AttributeList.

Diff Detail

Unit TestsFailed

TimeTest
270 msx64 debian > LLVM.Transforms/InstCombine::call-cast-attrs.ll
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/Transforms/InstCombine/call-cast-attrs.ll -instcombine -S | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/Transforms/InstCombine/call-cast-attrs.ll
150 msx64 windows > LLVM.Transforms/InstCombine::call-cast-attrs.ll
Script: -- : 'RUN: at line 1'; c:\ws\w2\llvm-project\premerge-checks\build\bin\opt.exe < C:\ws\w2\llvm-project\premerge-checks\llvm\test\Transforms\InstCombine\call-cast-attrs.ll -instcombine -S | c:\ws\w2\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w2\llvm-project\premerge-checks\llvm\test\Transforms\InstCombine\call-cast-attrs.ll

Event Timeline

gchatelet created this revision.Jul 6 2021, 7:59 AM
gchatelet requested review of this revision.Jul 6 2021, 7:59 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript

The gist of the patch is the addition of a inner Index class inside AttributeList see llvm/include/llvm/IR/Attributes.h.
This patch should be NFC.

gchatelet updated this revision to Diff 356728.Jul 6 2021, 8:04 AM
  • revert unrelated change
foad added inline comments.Jul 6 2021, 8:16 AM
llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp
641

This is just a drive-by comment, so feel free to ignore, but for client code like this it would be nicer if we could just write AttributeList::getArgIndex. (It already says Index in the function name so why do I have to qualify it with Index:: as well?)

gchatelet updated this revision to Diff 356736.Jul 6 2021, 8:19 AM
  • rename functions
gchatelet added inline comments.Jul 6 2021, 8:22 AM
llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp
641

Yes good point, I'm still refining the API to make it easier to use. This is is great suggestion thx!

gchatelet updated this revision to Diff 356740.Jul 6 2021, 8:28 AM
  • Provide a simpler way to create Arg indices
gchatelet marked an inline comment as done.Jul 6 2021, 8:29 AM
gchatelet updated this revision to Diff 356883.Jul 7 2021, 12:34 AM
  • Rebase and Fix lint issues
gchatelet updated this revision to Diff 356966.Jul 7 2021, 8:09 AM
  • rebase to get rid of unrelated broken tests
courbet accepted this revision.Jul 8 2021, 1:50 AM

Nice!

llvm/include/llvm/IR/Attributes.h
444

constexpr ?

llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp
212

This was broken, right ?

This revision is now accepted and ready to land.Jul 8 2021, 1:50 AM
gchatelet marked 2 inline comments as done.Jul 8 2021, 6:22 AM
gchatelet added subscribers: cherry, anna, thanm.

@cherry @anna @thanm since you worked on D53602. Do you have a sense of whether the failing test is legitimate?

Basically these lines :

call void bitcast (void (...)* @c to void (i32*)*)(i32* sret(i32) %y)
call void bitcast (void (i32, ...)* @d to void (i32, i32*)*)(i32 0, i32* sret(i32) %y)

Are considered illegal by the Verifier and trigger the following assert:

if (TargetFuncType->isVarArg()) {
  AttributeSet ArgAttrs = Attrs.getParamAttributes(5 + i);
  Assert(!ArgAttrs.hasAttribute(Attribute::StructRet),
         "Attribute 'sret' cannot be used for vararg call arguments!",
         Call);
}

Any help here would be highly appreciated!

llvm/include/llvm/IR/Attributes.h
444

As discussed offline, I wasn't able to do so because the definition of AttributeList::Index::Index() is not available before AttributeList class is finished parsing.
This then fires the following error expression must have a constant value -- constexpr function "llvm::AttributeList::Index::Index(unsigned int Value)" (declared at line 394) is not definedC/C++(28)

llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp
212

This was broken, right ?

Yes I believe so. It might 'happen to work' by chance.

gchatelet marked 2 inline comments as done.Jul 8 2021, 6:22 AM
thanm added a comment.Jul 8 2021, 7:01 AM

I am confused as to why this test would be failing. The verifier fragment in question is for statepoints, and I don't see any statepoints in the test. When I run

./bin/opt -debug-pass-manager -instcombine -S < ../llvm-project/llvm/test/Transforms/InstCombine/call-cast-attrs.ll

I don't see anything related to statepoints or statepoint lowering.

I am confused as to why this test would be failing. The verifier fragment in question is for statepoints, and I don't see any statepoints in the test. When I run

./bin/opt -debug-pass-manager -instcombine -S < ../llvm-project/llvm/test/Transforms/InstCombine/call-cast-attrs.ll

I don't see anything related to statepoints or statepoint lowering.

I may have linked to the wrong line in Verifier.cpp. There are two asserts with the same message.

thanm added a comment.Jul 8 2021, 7:17 AM

I may have linked to the wrong line in Verifier.cpp. There are two asserts with the same message.

Ah, ok, that makes a bit more sense. https://reviews.llvm.org/D53602 really only deals with the statepoint case, so I am not sure if I have any insights as to what might be doing wrong with this test.

I may have linked to the wrong line in Verifier.cpp. There are two asserts with the same message.

Ah, ok, that makes a bit more sense. https://reviews.llvm.org/D53602 really only deals with the statepoint case, so I am not sure if I have any insights as to what might be doing wrong with this test.

My bad then. Sorry for bothering.

gchatelet added a subscriber: thanm.Jul 8 2021, 8:07 AM

I may have linked to the wrong line in Verifier.cpp. There are two asserts with the same message.

Ah, ok, that makes a bit more sense. https://reviews.llvm.org/D53602 really only deals with the statepoint case, so I am not sure if I have any insights as to what might be doing wrong with this test.

@thanm upon closer look the second error message also comes from D53602.
See https://reviews.llvm.org/D53602#change-oHfYVs0pQmdt

https://github.com/llvm/llvm-project/blob/ba913b8da57dcdcda0572ec3a6b8d4e367f22803/llvm/lib/IR/Verifier.cpp#L3267-L3272

// Statepoint intrinsic is vararg but the wrapped function may be not.
// Allow sret here and check the wrapped function in verifyStatepoint.
if (!Call.getCalledFunction() ||
    Call.getCalledFunction()->getIntrinsicID() !=
        Intrinsic::experimental_gc_statepoint)
  Assert(!ArgAttrs.hasAttribute(Attribute::StructRet),
         "Attribute 'sret' cannot be used for vararg call arguments!",
         Call);

So it's definitely one of these two (there are no other such error messages in the codebase)

Replying here to my for future self. There's currently a hole in the type system which lets AttributeIndex be created from an unsigned, this leaves a whole lot of indexing logic deal with the unsafe type, starting with llvm/utils/TableGen/IntrinsicEmitter.cpp.
So there is actually much more work to introduce a proper type.

I'm putting this patch on hold for now.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2681

The mistake is here it should be SRetIdx.rawValue() > FT->getNumParams() but this leaks the abstraction.

gchatelet planned changes to this revision.Jul 22 2021, 5:45 AM