[LSR] Pointers with different address spaces are considered incompatible.

[uabelho]

Summary

Function isCompatibleIVType is already used as a guard before the call to

SE.getMinusSCEV(OperExpr, PrevExpr);

in LSRInstance::ChainInstruction. getMinusSCEV requires the expressions
to be of the same type, so we now consider two pointers with different
address spaces to be incompatible, since it is possible that the pointers
in fact have different sizes.

[uabelho]

Very similar to https://reviews.llvm.org/D29717 but in LSR instead of LoopVectorize.

As I wrote in the commit message I haven't been able to reproduce this on an in-tree
target. If anyone knows a target where pointers in different address spaces have different
sizes, then I can probably fix a reproducer exposing a failed assertion in SCEV.

[uabelho]

After a hint from my colleague Ka-Ka (thanks!) I managed to reproduce the crash on "amdgcn--".

I'll update the patch with a test case tomorrow.

[uabelho]

Added test case. Without the fix the test case makes the compiler crash in SE.getMinusSCEV(OperExpr, PrevExpr);

[qcolombet]

LGTM

llvm/trunk/lib/Transforms/Scalar/LoopStrengthReduce.cpp

if (TruncInst *Trunc = dyn_cast<TruncInst>(Oper))
     return Trunc->getOperand(0);
   return Oper;
 }
 
 /// Return true if we allow an IV chain to include both types.
 static bool isCompatibleIVType(Value *LVal, Value *RVal) {
   Type *LType = LVal->getType();
   Type *RType = RVal->getType();
-  return (LType == RType) || (LType->isPointerTy() && RType->isPointerTy());
+  return (LType == RType) || (LType->isPointerTy() && RType->isPointerTy() &&
+                              // Different address spaces means (possibly)
+                              // different types of the pointer implementation,
+                              // e.g. i16 vs i32 so disallow that.
+                              (LType->getPointerAddressSpace() ==
+                               RType->getPointerAddressSpace()));
 }
 
 /// Return an approximation of this SCEV expression's "base", or NULL for any
 /// constant. Returning the expression itself is conservative. Returning a
 /// deeper subexpression is more precise and valid as long as it isn't less
 /// complex than another subexpression. For expressions involving multiple
 /// unscaled values, we need to return the pointer-type SCEVUnknown. This avoids
 /// forming chains across objects, such as: PrevOper==a[i], IVOper==b[i],

llvm/trunk/test/Transforms/LoopStrengthReduce/AMDGPU/different-addrspace-crash.ll

+; RUN: llc < %s | FileCheck %s
+
+target datalayout = "e-p:32:32-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5: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"
+target triple = "amdgcn--"
+
+; We need to compile this for a target where we have different address spaces,
+; and where pointers in those address spaces have different size.
+; E.g. for amdgcn-- pointers in address space 0 are 32 bits and pointers in
+; address space 1 are 64 bits.
+
+; We shouldn't crash. Check that we get a loop with the two stores.
+;CHECK-LABEL: foo:
+;CHECK: [[LOOP_LABEL:BB[0-9]+_[0-9]+]]:
+;CHECK: buffer_store_dword
+;CHECK: buffer_store_dword
+;CHECK: s_branch [[LOOP_LABEL]]
+
+define void @foo() {
+entry:
+  br label %loop
+
+loop:
+  %idx0 = phi i32 [ %next_idx0, %loop ], [ 0, %entry ]
+  %0 = getelementptr inbounds i32, i32* null, i32 %idx0
+  %1 = getelementptr inbounds i32, i32 addrspace(1)* null, i32 %idx0
+  store i32 1, i32* %0
+  store i32 7, i32 addrspace(1)* %1
+  %next_idx0 = add nuw nsw i32 %idx0, 1
+  br label %loop
+}
+