[AlignmentFromAssumptions] Fix a SCEV assertion resulting from address space differences.

[DiamondLovesYou]

Summary

On targets with different pointer sizes, -alignment-from-assumptions could attempt to create SCEV expressions which use different effective SCEV types. The provided test illustrates the issue.

In getNewAlignment, AASCEV would be the (only) alloca, which would have an effective SCEV type of i32. But PtrSCEV, the GEP in this case, due to being in the flat/default address space, will have an effective SCEV of i64.

This patch resolves the issue by truncating PtrSCEV to AASCEV's effective type.

[DiamondLovesYou]

Fix CI issue ("no CHECK line").

[jdoerfert]

I think this is generally fine, except the test case changes noted below.

FWIW, this pass will be deleted (hopefully soon).

llvm/test/Transforms/AlignmentFromAssumptions/amdgpu-crash.ll

4

If you want to test for "non-crash", remove the FileCheck. However, I think you want to test that the generated expression is actually as expected instead.

48

Please remove all these attributes and metadata notes, etc., none of it should be needed.

[jdoerfert]

Is this still active?

[DiamondLovesYou]

Clean up the test.

[DiamondLovesYou]

Sorry for the delay; yes this is still active.

llvm/test/Transforms/AlignmentFromAssumptions/amdgpu-crash.ll

4

If you want to test for "non-crash", remove the FileCheck.

Ah, should have known, thanks.

I think you want to test that the generated expression is actually as expected instead.

There's nothing to test (aside from not crashing): -alignment-from-assumptions doesn't actually change anything in this test, even if I remove the alignment on the store or up the alignment from the assumption and alloca.

[jdoerfert]

LGTM.

[alex-t]

This change does not fix a real problem but covers it instead.
Moreover, the test, which is assumed to crash without the change does not test anything as the @llvm.assume call has no bundles, and the
AlignmentFromAssumptionsPass::runImpl main loop never executes processAssumption.
Also, since the 615efc3ed59cd5073127e4f0709ae35e58a5dc4f [AlignmentFromAssumptions] Migrate tests to opaque pointers the test does not make sense at all as both
PtrSCEV and AASCEV are always just generic pointers.

It looks strange to me that nobody from the AMDGPU backend developers was invited to review this.

[alex-t]

llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp

133

What will be if the PtrSCEV is 32bit but the AASCEV is 64bit?
You cannot zero extend a pointer type.

llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp

 
 // There is an address given by an offset OffSCEV from AASCEV which has an
 // alignment AlignSCEV. Use that information, if possible, to compute a new
 // alignment for Ptr.
 static unsigned getNewAlignment(const SCEV *AASCEV, const SCEV *AlignSCEV,
                                 const SCEV *OffSCEV, Value *Ptr,
                                 ScalarEvolution *SE) {
   const SCEV *PtrSCEV = SE->getSCEV(Ptr);
+  // On a platform with 32-bit allocas, but 64-bit flat/global pointer sizes
+  // (*cough* AMDGPU), the effective SCEV type of AASCEV and PtrSCEV
+  // may disagree. Trunc/extend so they agree.
+  PtrSCEV = SE->getTruncateOrZeroExtend(
+      PtrSCEV, SE->getEffectiveSCEVType(AASCEV->getType()));
   const SCEV *DiffSCEV = SE->getMinusSCEV(PtrSCEV, AASCEV);
 
   // On 32-bit platforms, DiffSCEV might now have type i32 -- we've always
   // sign-extended OffSCEV to i64, so make sure they agree again.
   DiffSCEV = SE->getNoopOrSignExtend(DiffSCEV, OffSCEV->getType());
 
   // What we really want to know is the overall offset to the aligned
   // address. This address is displaced by the provided offset.

llvm/test/Transforms/AlignmentFromAssumptions/amdgpu-crash.ll

+; Test that we don't crash.
+; RUN: opt < %s -alignment-from-assumptions -S
+; RUN: opt < %s -passes=alignment-from-assumptions -S
+
+target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-ni:7"
+
+%"core::str::CharIndices.29.66.90.114.138.149.165.173.181.197.205.213.229.387.398" = type { [0 x i64], i64, [0 x i64], { i8*, i8* }, [0 x i64] }
+%"unwind::libunwind::_Unwind_Exception.9.51.75.99.123.147.163.171.179.195.203.211.227.385.396" = type { [0 x i64], i64, [0 x i64], void (i32, %"unwind::libunwind::_Unwind_Exception.9.51.75.99.123.147.163.171.179.195.203.211.227.385.396"*)*, [0 x i64], [6 x i64], [0 x i64] }
+%"unwind::libunwind::_Unwind_Context.10.52.76.100.124.148.164.172.180.196.204.212.228.386.397" = type { [0 x i8] }
+
+define void @"_ZN44_$LT$$RF$T$u20$as$u20$core..fmt..Display$GT$3fmt17h7b1d039c7ff5e1feE"() {
+start:
+  %_15.i.i = alloca %"core::str::CharIndices.29.66.90.114.138.149.165.173.181.197.205.213.229.387.398", align 8, addrspace(5)
+  br label %bb12.i.i
+
+bb12.i.i:                                         ; preds = %start
+  %0 = addrspacecast %"core::str::CharIndices.29.66.90.114.138.149.165.173.181.197.205.213.229.387.398" addrspace(5)* %_15.i.i to %"core::str::CharIndices.29.66.90.114.138.149.165.173.181.197.205.213.229.387.398"*
+  %ptrint53.i.i = ptrtoint %"core::str::CharIndices.29.66.90.114.138.149.165.173.181.197.205.213.229.387.398"* %0 to i64
+  %maskedptr54.i.i = and i64 %ptrint53.i.i, 7
+  %maskcond55.i.i = icmp eq i64 %maskedptr54.i.i, 0
+  call void @llvm.assume(i1 %maskcond55.i.i)
+  br i1 undef, label %bb20.i.i, label %bb3.i.i.i.i.i.preheader.i.i
+
+bb3.i.i.i.i.i.preheader.i.i:                      ; preds = %bb12.i.i
+  %1 = getelementptr inbounds %"core::str::CharIndices.29.66.90.114.138.149.165.173.181.197.205.213.229.387.398", %"core::str::CharIndices.29.66.90.114.138.149.165.173.181.197.205.213.229.387.398"* %0, i64 0, i32 0, i64 0
+  store i64 0, i64* %1, align 8
+  unreachable
+
+bb20.i.i:                                         ; preds = %bb12.i.i
+  ret void
+}
+
+declare void @llvm.assume(i1)