[BPF] Fix BitCast Assertion with NonZero AddrSpace #130722
Conversation
Alexei reported a bpf selftest failure with recent llvm for bpf prog
file progs/arena_spin_lock.c. The failure only happens when clang
is built with cmake option LLVM_ENABLE_ASSERTIONS=ON.
The error message looks like:
clang: /home/yhs/work/yhs/llvm-project/llvm/lib/IR/Instructions.cpp:3460:
llvm::BitCastInst::BitCastInst(Value *, Type *, const Twine &, InsertPosition):
Assertion `castIsValid(getOpcode(), S, Ty) && "Illegal BitCast"' failed.
Further investigation shows that the problem is triggered in
BPF/BPFAbstractMemberAccess.cpp
for code
auto *BCInst =
new BitCastInst(Base, PointerType::getUnqual(BB->getContext()));
For the above BitCastInst, Since 'Base' has non-zero AddrSapce,
the compiler expects the type also has the same AddrSpace. But the
above PointerType::getUnqual(...) does not have AddrSpace and hence
causes the assertion failure.
Providing the proper AddrSpace for the BitCast type fixed the issue.
|
tested the fix. Works. |
There was a problem hiding this comment.
The change looks good to me. At cursory glance I don't see other points in this pass that might require similar change.
I have a nit about the test: for IR to IR passes it is a general practice to run opt -passes=<the-pass> and verify the output. But all our tests for CORE seem to follow a different pattern, there is even no way to request BPFAbstractMemberAccess from opt, a change in the BPFPassRegistry.def is needed for that. So this is a moot point.
Yes, I was actually aware of this when I tried to write the test. As you mentioned, we do not add a function pass for that. The main reason is that this test is to test compilation failure, so I didn't add func test. Yes, previously we mostly care the final result, at least for people who understands final asm code but they may not be really familiar with IR. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/1090 Here is the relevant piece of the build log for the reference |
| new BitCastInst(Base, PointerType::getUnqual(BB->getContext())); | ||
| auto *BCInst = new BitCastInst( | ||
| Base, PointerType::get(BB->getContext(), | ||
| Base->getType()->getPointerAddressSpace())); |
There was a problem hiding this comment.
Can't you drop this bitcast and the one below entirely? They are not needed anymore with opaque pointers.
There was a problem hiding this comment.
Okay, let me give a try.
There was a problem hiding this comment.
Did this change make it into the backport: #130995
There was a problem hiding this comment.
Since backport is only for bug fix, I think the suggestion to drop the above bitcast does not need to be backported since it does not fix any bug. I hope my understanding is correct. But for llvm21, I will make proper changes to remove these bitcasts.
In [1], Nikita Popov spotted that two BitCast operations are not needed with opaque pointers. So remove these two BitCast operations and adjust corresponding comments as well. [1] llvm#130722
In [1], Nikita Popov spotted that two BitCast operations are not needed with opaque pointers. So remove these two BitCast operations and adjust corresponding comments as well. [1] #130722 Co-authored-by: Yonghong Song <[email protected]>
In [1], Nikita Popov spotted that two BitCast operations are not needed with opaque pointers. So remove these two BitCast operations and adjust corresponding comments as well. [1] llvm/llvm-project#130722 Co-authored-by: Yonghong Song <[email protected]>
Alexei reported a bpf selftest failure with recent llvm for bpf prog file progs/arena_spin_lock.c. The failure only happens when clang is built with cmake option LLVM_ENABLE_ASSERTIONS=ON.
The error message looks like:
Further investigation shows that the problem is triggered in
BPF/BPFAbstractMemberAccess.cpp
for code
For the above BitCastInst, Since 'Base' has non-zero AddrSapce, the compiler expects the type also has the same AddrSpace. But the above PointerType::getUnqual(...) does not have AddrSpace and hence causes the assertion failure.
Providing the proper AddrSpace for the BitCast type fixed the issue.