Verify threadlocal_address constraints #87841
Conversation
Check invariants for `llvm.threadlocal.address` intrinsic in IR Verifier.
|
@AlexVlx the I'm not really sure whether this is a desirable pattern, if so then we should document this as an allowed use case for the |
|
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-llvm-ir Author: Matthias Braun (MatzeB) ChangesCheck invariants for Full diff: https://github.com/llvm/llvm-project/pull/87841.diff 1 Files Affected:
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 64c59914cf2fc2..ac3d13f3d2b057 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -6223,6 +6223,14 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
&Call);
break;
}
+ case Intrinsic::threadlocal_address: {
+ const Value &Arg0 = *Call.getArgOperand(0);
+ Check(isa<GlobalVariable>(Arg0),
+ "llvm.threadlocal.address first argument must be a GlobalVariable");
+ Check(cast<GlobalVariable>(Arg0).isThreadLocal(),
+ "llvm.threadlocal.address operand isThreadLocal() must no be false");
+ break;
+ }
};
// Verify that there aren't any unmediated control transfers between funclets.
|
|
Hmm, that is an interesting question; whilst I recall having excised this from an actual app, it does look somewhat contrived / bogus looking at it with today's eyes. I think it's fine to |
|
It seems code like my #87844 is slightly simpler when it can rely on there only ever being a That said: If this pattern is helpful to support then I can trivially add some loop to skip address space casts in #87844 instead, so this isn't really blocking anything. I was just wondering what the rules are... |
MatzeB
force-pushed
the
thread_local_verify
branch
from
30d1900
to
f923274
Compare
There was a problem hiding this comment.
LGTM. I am a bit concerned that we're going to violate these rules in some transforms (e.g. by creating threadlocal.address of a phi or select), though at least from some cursory testing we seem to already avoid converting intrinsics with constants args into non-constants. I guess we can try this and see whether anything falls out.
Me too, but that is all the more reason to have this verifier IMO |
MatzeB
force-pushed
the
thread_local_verify
branch
from
8d686b1
to
05e6eed
Compare
|
This seems to have caused a regression when integrating into emscripten today: https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8751138098141239841/+/u/Build_Emscripten/stdout When I run locally I get this: See: |
|
Is this using an unmodified clang/llvm? Is there a way to reproduce on clang trunk (ideally without doing a full chromium build)? |
|
Yes, this is unmodified llvm. No need to build chromium. This looks like a Wasm specific issue. It can be reproduced by simply compiling https://github.com/emscripten-core/emscripten/blob/main/system/lib/libc/emscripten_time.c with emscripten. To simplify this I've attached the pre-processed version of that file which you can reproduce this crash with using just: |
|
So this code clears the It happens to work anyway by accident, but needs to adjust all users now... let's see how hard that is... |
|
NVM: This might be caused by some custom modifications on our end that lead to illegal intrinsics. |
|
I just noticed that LLVM allows thread-local |
Remove `llvm.threadlocal.address` intrinsic usage when disabling TLS. This fixes errors revealed by the stricter IR verification introduced in PR #87841.
Check invariants for
llvm.threadlocal.addressintrinsic in IR Verifier.