This is a follow up patch on https://reviews.llvm.org/D32811.
It avoids leaking information about the encoding of the index for
AttributeList.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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.
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?) |
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! |
@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. | |
llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp | ||
212 |
Yes I believe so. It might 'happen to work' by chance. |
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.
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
// 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. |
constexpr ?