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

4561–4565

No, we should emit it as a simple assumption.

llvm/lib/IR/Verifier.cpp

4611–4619

@jdoerfert Should second/third arguments be a *constant* integers ?

[rmansfield]

clang/lib/CodeGen/CGCall.cpp

4561–4565

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

4561–4565

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

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

4561–4565

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; // We're done. Disallow doing anything else.
       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;
   }
 

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);
 }

llvm/lib/IR/Verifier.cpp

for (auto &Elem : Call.bundle_op_infos()) {
       Attribute::AttrKind Kind =
           Attribute::getAttrKindFromName(Elem.Tag->getKey());
       unsigned ArgCount = Elem.End - Elem.Begin;
       if (Kind == Attribute::Alignment) {
         Assert(ArgCount <= 3 && ArgCount >= 2,
                "alignment assumptions should have 2 or 3 arguments");
         Assert(Call.getOperand(Elem.Begin)->getType()->isPointerTy(),
                "first argument should be a pointer");
         Assert(Call.getOperand(Elem.Begin + 1)->getType()->isIntegerTy(),
                "second argument should be an integer");
+  ConstantInt *Alignment = dyn_cast<ConstantInt>(Call.getOperand(Elem.Begin + 1));
+  if (Alignment)
+    Assert(Alignment->getValue().isPowerOf2(),
+           "alignment must be a power of 2", Call);
         if (ArgCount == 3)
           Assert(Call.getOperand(Elem.Begin + 2)->getType()->isIntegerTy(),
                  "third argument should be an integer if present");
         return;
       }
-      Assert(ArgCount <= 2, "to many arguments");
+      Assert(ArgCount <= 2, "too many arguments");
       if (Kind == Attribute::None)
         break;
       if (Attribute::isIntAttrKind(Kind)) {
         Assert(ArgCount == 2, "this attribute should have 2 arguments");
         Assert(isa<ConstantInt>(Call.getOperand(Elem.Begin + 1)),
                "the second argument should be a constant integral value");
       } else if (Attribute::canUseAsParamAttr(Kind)) {
         Assert((ArgCount) == 1, "this attribute should have one argument");

llvm/test/Verifier/assume-bundles.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: not opt -verify < %s 2>&1 | FileCheck %s
 
 declare void @llvm.assume(i1)
 
 define void @func(i32* %P, i32 %P1, i32* %P2, i32* %P3) {
 ; CHECK: tags must be valid attribute names
   call void @llvm.assume(i1 true) ["adazdazd"()]
 ; CHECK: the second argument should be a constant integral value
   call void @llvm.assume(i1 true) ["dereferenceable"(i32* %P, i32 %P1)]
-; CHECK: to many arguments
+; CHECK: too many arguments
   call void @llvm.assume(i1 true) ["dereferenceable"(i32* %P, i32 8, i32 8)]
 ; CHECK: this attribute should have 2 arguments
   call void @llvm.assume(i1 true) ["dereferenceable"(i32* %P)]
 ; CHECK: this attribute has no argument
   call void @llvm.assume(i1 true) ["dereferenceable"(i32* %P, i32 4), "cold"(i32* %P)]
 ; CHECK: this attribute should have one argument
   call void @llvm.assume(i1 true) ["noalias"()]
   call void @llvm.assume(i1 true) ["align"(i32* %P, i32 %P1, i32 4)]
 ; CHECK: alignment assumptions should have 2 or 3 arguments
   call void @llvm.assume(i1 true) ["align"(i32* %P, i32 %P1, i32 4, i32 4)]
 ; CHECK: second argument should be an integer
   call void @llvm.assume(i1 true) ["align"(i32* %P, i32* %P2)]
+; CHECK: alignment must be a power of 2
+  call void @llvm.assume(i1 true) ["align"(i32* %P, i32 7)]
 ; CHECK: third argument should be an integer if present
   call void @llvm.assume(i1 true) ["align"(i32* %P, i32 %P1, i32* %P2)]
   ret void
 }