[InstructionSimplify] Apply sext/trunc after pointer stripping

[hliao]

Summary

• As the pointer stripping could trace through addrspacecast now, need to sext/trunc the offset to ensure it has the same width as the pointer after stripping.

[hliao]

this fixes crashes found on targets where addrspacecast is used.

[jdoerfert]

This is fine with me. Though, I'm unsure if this should not live in the stripAndAccumulateConstantOffsets method.

The problem is that we have two bit-widths now, pointer before and after. Since we have to express offset in one of them, we have a choice.
I think we already guarantee that the offset is representable in the initial bit-width so that was what I choose. Question is, do we want
to change it to be in the resulting bit-width or not. Either way, we should add a comment to the stripAndAccumulateConstantOffsets method
explaining the situation.

[hliao]

In D64768#1586435, @jdoerfert wrote:

This is fine with me. Though, I'm unsure if this should not live in the stripAndAccumulateConstantOffsets method.

The problem is that we have two bit-widths now, pointer before and after. Since we have to express offset in one of them, we have a choice.
I think we already guarantee that the offset is representable in the initial bit-width so that was what I choose. Question is, do we want
to change it to be in the resulting bit-width or not. Either way, we should add a comment to the stripAndAccumulateConstantOffsets method
explaining the situation.

As I mentioned, without this fix, there are crashes when compiling similar patterns. The issue is that LHS gets the original bit-width as the offset is 0 and RHS gets the one after stripping. Later, as the stripped pointers point the same object, instsimplify will create compare of the offset, that triggers the crash. We have to ensure offset after stripping has the same bit-width.

[jdoerfert]

LGTM, one request and a proposal to fix it slightly differently.

In D64768#1586555, @hliao wrote:

In D64768#1586435, @jdoerfert wrote:

This is fine with me. Though, I'm unsure if this should not live in the stripAndAccumulateConstantOffsets method.

The problem is that we have two bit-widths now, pointer before and after. Since we have to express offset in one of them, we have a choice.
I think we already guarantee that the offset is representable in the initial bit-width so that was what I choose. Question is, do we want
to change it to be in the resulting bit-width or not. Either way, we should add a comment to the stripAndAccumulateConstantOffsets method
explaining the situation.

As I mentioned, without this fix, there are crashes when compiling similar patterns. The issue is that LHS gets the original bit-width as the offset is 0 and RHS gets the one after stripping. Later, as the stripped pointers point the same object, instsimplify will create compare of the offset, that triggers the crash. We have to ensure offset after stripping has the same bit-width.

I understand. That is what I tried to describe above. My question was: Should stripAndAccumulateConstantOffsets return the offset wrt. the bit-width of the input pointer or output pointer? I can see arguments for both. If you think this way is better you can go ahead and commit. If you think the other way around is better, we should move the conversion into stripAndAccumulateConstantOffsets. I personally tend towards the latter.

Either way, we have to add a comment to the stripAndAccumulateConstantOffsets method explaining the choice!

[hliao]

In D64768#1586690, @jdoerfert wrote:

LGTM, one request and a proposal to fix it slightly differently.

In D64768#1586555, @hliao wrote:

In D64768#1586435, @jdoerfert wrote:

This is fine with me. Though, I'm unsure if this should not live in the stripAndAccumulateConstantOffsets method.

The problem is that we have two bit-widths now, pointer before and after. Since we have to express offset in one of them, we have a choice.
I think we already guarantee that the offset is representable in the initial bit-width so that was what I choose. Question is, do we want
to change it to be in the resulting bit-width or not. Either way, we should add a comment to the stripAndAccumulateConstantOffsets method
explaining the situation.

As I mentioned, without this fix, there are crashes when compiling similar patterns. The issue is that LHS gets the original bit-width as the offset is 0 and RHS gets the one after stripping. Later, as the stripped pointers point the same object, instsimplify will create compare of the offset, that triggers the crash. We have to ensure offset after stripping has the same bit-width.

I understand. That is what I tried to describe above. My question was: Should stripAndAccumulateConstantOffsets return the offset wrt. the bit-width of the input pointer or output pointer? I can see arguments for both. If you think this way is better you can go ahead and commit. If you think the other way around is better, we should move the conversion into stripAndAccumulateConstantOffsets. I personally tend towards the latter.

Either way, we have to add a comment to the stripAndAccumulateConstantOffsets method explaining the choice!

I prefer the offset to output pointer as that's that API is designed for, strip any constant adjustments. If the final pointer has different bit-width, we'd better prepare that offset matching that pointer's integer width.

llvm/trunk/lib/Analysis/InstructionSimplify.cpp

 static Constant *stripAndComputeConstantOffsets(const DataLayout &DL, Value *&V,
                                                 bool AllowNonInbounds = false) {
   assert(V->getType()->isPtrOrPtrVectorTy());
 
   Type *IntPtrTy = DL.getIntPtrType(V->getType())->getScalarType();
   APInt Offset = APInt::getNullValue(IntPtrTy->getIntegerBitWidth());
 
   V = V->stripAndAccumulateConstantOffsets(DL, Offset, AllowNonInbounds);
+  // As that strip may trace through `addrspacecast`, need to sext or trunc
+  // the offset calculated.
+  IntPtrTy = DL.getIntPtrType(V->getType())->getScalarType();
+  Offset = Offset.sextOrTrunc(IntPtrTy->getIntegerBitWidth());
 
   Constant *OffsetIntPtr = ConstantInt::get(IntPtrTy, Offset);
   if (V->getType()->isVectorTy())
     return ConstantVector::getSplat(V->getType()->getVectorNumElements(),
                                     OffsetIntPtr);
   return OffsetIntPtr;
 }
 

llvm/trunk/test/Transforms/InstSimplify/compare.ll

 ; RUN: opt < %s -instsimplify -S | FileCheck %s
-target datalayout = "p:32:32"
+target datalayout = "p:32:32-p1:64:64"
 
 define i1 @ptrtoint() {
 ; CHECK-LABEL: @ptrtoint(
   %a = alloca i8
   %tmp = ptrtoint i8* %a to i32
   %r = icmp eq i32 %tmp, 0
   ret i1 %r
 ; CHECK: ret i1 false

 define i1 @constant_fold_null_inttoptr() {
 ; CHECK-LABEL: @constant_fold_null_inttoptr(
 ; CHECK-NEXT:    ret i1 false
 ;
   %x = icmp eq i32* null, inttoptr (i64 32 to i32*)
   ret i1 %x
 }
 
+; CHECK-LABEL: @cmp_through_addrspacecast(
+; CHECK-NEXT: ret i1 true
+define i1 @cmp_through_addrspacecast(i32 addrspace(1)* %p1) {
+  %p0 = addrspacecast i32 addrspace(1)* %p1 to i32*
+  %p0.1 = getelementptr inbounds i32, i32* %p0, i64 1
+  %cmp = icmp ne i32* %p0, %p0.1
+  ret i1 %cmp
+}
+
 attributes #0 = { "null-pointer-is-valid"="true" }