[AlignmentFromAssumptions] avoid crash on alignment constant expression

[spatel]

Summary

I'm not familiar with this pass, but we have a crash shown in:
https://llvm.org/PR50978
...so I think we can either bail out as shown here or dyn_cast later to avoid the assert failure in getNewAlignmentDiff() that happens with the plain cast<SCEVConstant>.

[jdoerfert]

I still believe we should not create alignment annotations for non-power-of-two constants. This is the second time in a few weeks this causes problems and we still play whack-a-mole instead of restricting clang and teaching the verifier.

https://reviews.llvm.org/D94433

[spatel]

In D106368#2890603, @jdoerfert wrote:

I still believe we should not create alignment annotations for non-power-of-two constants. This is the second time in a few weeks this causes problems and we still play whack-a-mole instead of restricting clang and teaching the verifier.

https://reviews.llvm.org/D94433

Ah, I didn't realize this is the same problem based on the reduced test. Someone familiar with this feature in Clang -- and compatibility with GCC? -- should have a look then ( @lebedev.ri @erichkeane ? ) - I have no idea what this is supposed to do.

[lebedev.ri]

In D106368#2890779, @spatel wrote:

In D106368#2890603, @jdoerfert wrote:

I still believe we should not create alignment annotations for non-power-of-two constants. This is the second time in a few weeks this causes problems and we still play whack-a-mole instead of restricting clang and teaching the verifier.

https://reviews.llvm.org/D94433

Ah, I didn't realize this is the same problem based on the reduced test. Someone familiar with this feature in Clang -- and compatibility with GCC? -- should have a look then ( @lebedev.ri @erichkeane ? ) - I have no idea what this is supposed to do.

We (clang) already diagnoses it, but doesn't refuse to codegen it.
I think it should just hard-error on it, without emitting IR.

[rmansfield]

In D106368#2890810, @lebedev.ri wrote:

In D106368#2890779, @spatel wrote:

In D106368#2890603, @jdoerfert wrote:

I still believe we should not create alignment annotations for non-power-of-two constants. This is the second time in a few weeks this causes problems and we still play whack-a-mole instead of restricting clang and teaching the verifier.

https://reviews.llvm.org/D94433

Ah, I didn't realize this is the same problem based on the reduced test. Someone familiar with this feature in Clang -- and compatibility with GCC? -- should have a look then ( @lebedev.ri @erichkeane ? ) - I have no idea what this is supposed to do.

We (clang) already diagnoses it, but doesn't refuse to codegen it.
I think it should just hard-error on it, without emitting IR.

Wouldn't that be revert of https://reviews.llvm.org/D73996 which softened it to a warning then?

[lebedev.ri]

In D106368#2922582, @rmansfield wrote:

In D106368#2890810, @lebedev.ri wrote:

In D106368#2890779, @spatel wrote:

In D106368#2890603, @jdoerfert wrote:

I still believe we should not create alignment annotations for non-power-of-two constants. This is the second time in a few weeks this causes problems and we still play whack-a-mole instead of restricting clang and teaching the verifier.

https://reviews.llvm.org/D94433

Ah, I didn't realize this is the same problem based on the reduced test. Someone familiar with this feature in Clang -- and compatibility with GCC? -- should have a look then ( @lebedev.ri @erichkeane ? ) - I have no idea what this is supposed to do.

We (clang) already diagnoses it, but doesn't refuse to codegen it.
I think it should just hard-error on it, without emitting IR.

Wouldn't that be revert of https://reviews.llvm.org/D73996 which softened it to a warning then?

Oh, i forgot that i had such a patch.
That seems to list a reason why clang does what it does,
I guess then we need two changes:

1. don't use alignment assume bundle to store non-power-of-two alignment, because that is obviously not an alignment
2. verifier to enforce that alignment assume bundle must contain powers of two.

[kallisti5]

I should mention Haiku has seen this crash via https://github.com/llvm/llvm-project/issues/53159

• -O2 results in a crash
• -O0 fixes the crash
• Fixing the alignment to non-0 fixes the crash.

[nikic]

D119414 clarified that this is legal IR, so this LGTM now.

[nikic]

Actually, it looks like the same patch has already been committed in the meantime: https://github.com/llvm/llvm-project/commit/9b45fd909ffa754acbb4e927bc2d55c7ab0d4e3f

llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp

bool AlignmentFromAssumptionsPass::extractAlignmentInfo(CallInst *I,
   OperandBundleUse AlignOB = I->getOperandBundleAt(Idx);
   if (AlignOB.getTagName() != "align")
     return false;
   assert(AlignOB.Inputs.size() >= 2);
   AAPtr = AlignOB.Inputs[0].get();
   // TODO: Consider accumulating the offset to the base.
   AAPtr = AAPtr->stripPointerCastsSameRepresentation();
   AlignSCEV = SE->getSCEV(AlignOB.Inputs[1].get());
+  // We only handle a simple constant (constant expressions not allowed).
+  if (!isa<SCEVConstant>(AlignSCEV))
+    return false;
   AlignSCEV = SE->getTruncateOrZeroExtend(AlignSCEV, Int64Ty);
   if (AlignOB.Inputs.size() == 3)
     OffSCEV = SE->getSCEV(AlignOB.Inputs[2].get());
   else
     OffSCEV = SE->getZero(Int64Ty);
   OffSCEV = SE->getTruncateOrZeroExtend(OffSCEV, Int64Ty);
   return true;
 }

llvm/test/Transforms/AlignmentFromAssumptions/simple.ll

entry:
   tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %0, i8* align 4 %1, i64 64, i1 false)
   ret i32 undef
 
 ; CHECK-LABEL: @moo3
 ; CHECK: @llvm.memcpy.p0i8.p0i8.i64(i8* align 32 %0, i8* align 128 %1, i64 64, i1 false)
 ; CHECK: ret i32 undef
 }
 
+; Don't crash on constant expression.
+
+@g = global i32 0
+
+define void @PR50978(i32* %arg) {
+; CHECK-LABEL: @PR50978(
+; CHECK-NEXT:    call void @llvm.assume(i1 true) [ "align"(i32* [[ARG:%.*]], i64 zext (i32 ptrtoint (i32* @g to i32) to i64)) ]
+; CHECK-NEXT:    [[TMP1:%.*]] = load i32, i32* [[ARG]], align 4
+; CHECK-NEXT:    ret void
+;
+  call void @llvm.assume(i1 true) [ "align"(i32* %arg, i64 zext (i32 ptrtoint (i32* @g to i32) to i64)) ]
+  load i32, i32* %arg
+  ret void
+}
+
 declare void @llvm.assume(i1) nounwind
 
 declare void @llvm.memset.p0i8.i64(i8* nocapture, i8, i64, i1) nounwind
 declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture, i8* nocapture, i64, i1) nounwind