This is an archive of the discontinued LLVM Phabricator instance.

Do not emit non-power-of-2 alignment assume bundles (PR48713).
AbandonedPublic

Authored by rmansfield on Jan 11 2021, 10:36 AM.

Diff Detail

Event Timeline

rmansfield created this revision.Jan 11 2021, 10:36 AM
rmansfield requested review of this revision.Jan 11 2021, 10:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2021, 10:36 AM

Adding more potential reviewers based on D88669 (where this code was added).

Updated diff with context

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

spatel accepted this revision.Jul 7 2021, 5:23 AM

LGTM - we either need to update the LangRef or have this quick fix to avoid crashing.

llvm/test/Analysis/ValueTracking/pr48713.ll
1

We should add FileCheck and have the expected CHECK lines for this test.

This revision is now accepted and ready to land.Jul 7 2021, 5:23 AM

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.

jdoerfert requested changes to this revision.Jul 20 2021, 8:37 AM

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 revision now requires changes to proceed.Jul 20 2021, 8:39 AM
rmansfield updated this revision to Diff 364826.Aug 6 2021, 9:43 AM
rmansfield retitled this revision from [ValueTracking] Check that alignment is non-zero in computeKnownBitsFromAssume (PR48713). to Do not emit non-power-of-2 alignment assume bundles (PR48713)..
rmansfield edited the summary of this revision. (Show Details)

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
4561–4565 ↗(On Diff #364826)

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 ?

rmansfield added inline comments.Aug 6 2021, 12:17 PM
clang/lib/CodeGen/CGCall.cpp
4561–4565 ↗(On Diff #364826)

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?

lebedev.ri added inline comments.Aug 6 2021, 12:19 PM
clang/lib/CodeGen/CGCall.cpp
4561–4565 ↗(On Diff #364826)

Just remove AA = nullptr; // We're done. Disallow doing anything else. here and you'll see

jdoerfert resigned from this revision.Sep 9 2021, 8:16 AM

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.

This revision is now accepted and ready to land.Sep 9 2021, 8:16 AM
lebedev.ri requested changes to this revision.Sep 9 2021, 9:27 AM

@rmansfield reverse-ping, thanks.

This revision now requires changes to proceed.Sep 9 2021, 9:27 AM
rmansfield added inline comments.Sep 9 2021, 11:39 AM
clang/lib/CodeGen/CGCall.cpp
4561–4565 ↗(On Diff #364826)

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.

rmansfield edited the summary of this revision. (Show Details)

Separate clang and llvm changes. I'm still looking for clarification on what clang should be emitting in the non-power of case.

rmansfield abandoned this revision.Apr 5 2022, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 8:43 AM