[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