Details
Diff Detail
Event Timeline
Please upload patch with context:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
No doubt there is a problem here somewhere, not sure if this is the best fix though or just hides it one level deeper.
I wonder if align(0) in the input is not already broken.
I'm trying to see if the Clang ppl are interested in changing the behavior that I think is at fault here: https://godbolt.org/z/jo4fx7
When we warn, we should not create the alignment assumption because in IR things depend on the fact that it is a power of two.
If we modify clang we should teach the verifier about the test case above and introduce an assertion isPowerOfTwo(RK.ArgValue).
Any updates on changing clang to avoid this?
There's a recent comment on:
https://llvm.org/PR48713
...that suggests fuzzers have found their way to this crash too.
Wouldn't we still want this check in the optimizer given the text in LangRef?
"if the alignment is set to zero, the alignment of the function is set by the target to whatever it feels convenient":
https://llvm.org/docs/LangRef.html#global-variables
LGTM - we either need to update the LangRef or have this quick fix to avoid crashing.
| llvm/test/Analysis/ValueTracking/pr48713.ll | ||
|---|---|---|
| 1 ↗ | (On Diff #315878) | We should add FileCheck and have the expected CHECK lines for this test. |
I still believe clang should not generate non-power-of-two alignment in the assume (https://godbolt.org/z/zzd1hWc88). We don't allow non-power-of-two in almost any other place already, but here we somehow do.
The IR verifier needs to be updated, clang should issue an error, the webpage should mention the requirement (https://clang.llvm.org/docs/AttributeReference.html), and we would not needs this.
With D106368 it now is clear this is not an isolated problem and we should finally fix clang and the verifier not all uses in llvm that have reasonable assumptions about the IR.
This looks like it's moving in the right direction.
Please split this up into clang change and a verifier change.
| clang/lib/CodeGen/CGCall.cpp | ||
|---|---|---|
| 4541–4546 | No, we should emit it as a simple assumption. | |
| llvm/lib/IR/Verifier.cpp | ||
| 4611–4619 ↗ | (On Diff #364826) | @jdoerfert Should second/third arguments be a *constant* integers ? |
| clang/lib/CodeGen/CGCall.cpp | ||
|---|---|---|
| 4541–4546 | Thanks for the feedback. Sorry for misunderstanding, from earlier comments I thought that no llvm.assume should be emitted for non-power-of-2 alignments. Can you elaborate on what you mean by simple assumption should be emitted? | |
| clang/lib/CodeGen/CGCall.cpp | ||
|---|---|---|
| 4541–4546 | Just remove AA = nullptr; // We're done. Disallow doing anything else. here and you'll see | |
I'm fine with this approach. @lebedev.ri has some comments and then this should be ok to go.
| llvm/lib/IR/Verifier.cpp | ||
|---|---|---|
| 4611–4619 ↗ | (On Diff #364826) | Right now, I think so. I'd be fine if we later allow non-constants but that is a different story. |
| clang/lib/CodeGen/CGCall.cpp | ||
|---|---|---|
| 4541–4546 | Sorry I didn't understand this comment and I haven't had time to revisit it to fully understand. I'm confused because if I remove the AA = nullptr; there's no change to clang and it will continue to generate an assume that the IR verifier is now being changed to reject. e.g. in clang/test/CodeGen/non-power-of-2-alignment-assumptions.c alloc(7); would generate %call = call i8* @alloc(i32 7) call void @llvm.assume(i1 true) [ "align"(i8* %call, i64 7) ] which would now be backend error: e.g alignment must be a power of 2 call void @llvm.assume(i1 true) [ "align"(i8* %call, i64 7) ] in function t1 fatal error: error in backend: Broken function found, compilation aborted! If you could explain what you would expect clang to emit in this case, I think that would help me and I can have another look. | |
Separate clang and llvm changes. I'm still looking for clarification on what clang should be emitting in the non-power of case.