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

[rmansfield]

Summary

[spatel]

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

[spatel]

Please upload patch with context:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[rmansfield]

Updated diff with context

[jdoerfert]

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).

[spatel]

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]

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.

[jdoerfert]

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]

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.

[lebedev.ri]

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 ?

[rmansfield]

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?

[lebedev.ri]

clang/lib/CodeGen/CGCall.cpp

4541–4546

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

[jdoerfert]

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.

[lebedev.ri]

@rmansfield reverse-ping, thanks.

[rmansfield]

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.

[rmansfield]

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

clang/lib/CodeGen/CGCall.cpp

public:
   /// If we can, materialize the alignment as an attribute on return value.
   LLVM_NODISCARD llvm::AttributeList
   TryEmitAsCallSiteAttribute(const llvm::AttributeList &Attrs) {
     if (!AA || OffsetCI || CGF.SanOpts.has(SanitizerKind::Alignment))
       return Attrs;
     const auto *AlignmentCI = dyn_cast<llvm::ConstantInt>(Alignment);
     if (!AlignmentCI)
       return Attrs;
-    // We may legitimately have non-power-of-2 alignment here.
-    // If so, this is UB land, emit it via `@llvm.assume` instead.
-    if (!AlignmentCI->getValue().isPowerOf2())
+    // Non power-of-2 alignment is UB and rejected by the IR verifier.
+    if (!AlignmentCI->getValue().isPowerOf2()) {
+      AA = nullptr;
       return Attrs;
+    }
     llvm::AttributeList NewAttrs = maybeRaiseRetAlignmentAttribute(
         CGF.getLLVMContext(), Attrs,
         llvm::Align(
             AlignmentCI->getLimitedValue(llvm::Value::MaximumAlignment)));
     AA = nullptr; // We're done. Disallow doing anything else.
     return NewAttrs;
   }
 
   /// Emit alignment assumption.

clang/test/CodeGen/non-power-of-2-alignment-assumptions.c

 void t0(int align) {
   alloc(align);
 }
 // CHECK-LABEL: @t1(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:    [[ALIGN_ADDR:%.*]] = alloca i32, align 4
 // CHECK-NEXT:    store i32 [[ALIGN:%.*]], i32* [[ALIGN_ADDR]], align 4
 // CHECK-NEXT:    [[CALL:%.*]] = call i8* @alloc(i32 7)
-// CHECK-NEXT:    call void @llvm.assume(i1 true) [ "align"(i8* [[CALL]], i64 7) ]
 // CHECK-NEXT:    ret void
 //
 void t1(int align) {
   alloc(7);
 }
 // CHECK-LABEL: @t2(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:    [[ALIGN_ADDR:%.*]] = alloca i32, align 4
 // CHECK-NEXT:    store i32 [[ALIGN:%.*]], i32* [[ALIGN_ADDR]], align 4
 // CHECK-NEXT:    [[CALL:%.*]] = call align 8 i8* @alloc(i32 8)
 // CHECK-NEXT:    ret void
 //
 void t2(int align) {
   alloc(8);
 }