NewGVN: Handle addrspacecast

[arsenm]

Summary

The AllConstant check needs to be moved out of the if/else if chain to
avoid a test regression. The "there is no SimplifyZExt" comment puzzles me,
since there is SimplifyCastInst. Additionally, the Simplify* calls
seem to not see the operand as constant, so this needs to be tried if
the simplify failed.

[arsenm]

ping

[jdoerfert]

This generally makes sense to me. I left a comment as it would be preferable to split the two conceptual changes if we can test them separately.
I think we also need addrspacecast tests with different address spaces to show they are not accidentally merged.

lib/Transforms/Scalar/NewGVN.cpp

1181

The way I understand this: If you have a BitCast, or maybe even another instruction, for which the simplify failed but AllConstant is true, you won't get constant folding before, correct?

Would it be possible to show the problem even w/o AddrSpaceCast?

[fhahn]

lib/Transforms/Scalar/NewGVN.cpp

1171

The code here should pass in E->getOperand(0), instead of CI->getOperand(0): we want to pass in the leader for the operand, not the operand from the original IR. This way, there should be no need to move the AllConstant condition. If the leader of the CastInst is constant, we should already simplify it here.

Please note that there is a subtle underlying issue with the way simplifications on the original IR and the simplified leaders interact, but that needs to be addressed separately and I do not think CastInst should have any special treatment here. If anything, it may make it a bit harder to trigger the issue.

[arsenm]

Pass correct operand, don't move AllConstant

[fhahn]

LGTM. Could you also add a test where we need to pass in the leader of the operand? Something like having 2 loads from the same address would probably be enough.

[arsenm]

r362653

lib/Transforms/Scalar/NewGVN.cpp

if (isa<Constant>(E->getOperand(0)) ||
       if (const Expression *SimplifiedE = checkSimplificationResults(E, I, V))
         return SimplifiedE;
     }
   } else if (I->isBinaryOp()) {
     Value *V =
         SimplifyBinOp(E->getOpcode(), E->getOperand(0), E->getOperand(1), SQ);
     if (const Expression *SimplifiedE = checkSimplificationResults(E, I, V))
       return SimplifiedE;
-  } else if (auto *BI = dyn_cast<BitCastInst>(I)) {
+  } else if (auto *CI = dyn_cast<CastInst>(I)) {
     Value *V =
-        SimplifyCastInst(BI->getOpcode(), BI->getOperand(0), BI->getType(), SQ);
+        SimplifyCastInst(CI->getOpcode(), CI->getOperand(0), CI->getType(), SQ);
     if (const Expression *SimplifiedE = checkSimplificationResults(E, I, V))
       return SimplifiedE;
   } else if (isa<GetElementPtrInst>(I)) {
     Value *V = SimplifyGEPInst(
         E->getType(), ArrayRef<Value *>(E->op_begin(), E->op_end()), SQ);
     if (const Expression *SimplifiedE = checkSimplificationResults(E, I, V))
       return SimplifiedE;
-  } else if (AllConstant) {
+  }
+
+  if (AllConstant) {
     // We don't bother trying to simplify unless all of the operands
     // were constant.
     // TODO: There are a lot of Simplify*'s we could call here, if we
     // wanted to.  The original motivating case for this code was a
     // zext i1 false to i8, which we don't have an interface to
     // simplify (IE there is no SimplifyZExt).
 
     SmallVector<Constant *, 8> C;
case Instruction::Call:
       break;
     case Instruction::Store:
       E = performSymbolicStoreEvaluation(I);
       break;
     case Instruction::Load:
       E = performSymbolicLoadEvaluation(I);
       break;
     case Instruction::BitCast:
+    case Instruction::AddrSpaceCast:
       E = createExpression(I);
       break;
     case Instruction::ICmp:
     case Instruction::FCmp:
       E = performSymbolicCmpEvaluation(I);
       break;
     case Instruction::Add:
     case Instruction::FAdd:

test/Transforms/NewGVN/addrspacecast.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -newgvn -S | FileCheck %s
+
+define i32 addrspace(1)* @addrspacecast(i32* %ptr) {
+; CHECK-LABEL: @addrspacecast(
+; CHECK-NEXT:  block1:
+; CHECK-NEXT:    [[Z1:%.*]] = addrspacecast i32* [[PTR:%.*]] to i32 addrspace(1)*
+; CHECK-NEXT:    br label [[BLOCK2:%.*]]
+; CHECK:       block2:
+; CHECK-NEXT:    store i32 addrspace(1)* [[Z1]], i32 addrspace(1)** undef
+; CHECK-NEXT:    ret i32 addrspace(1)* [[Z1]]
+;
+block1:
+  %z1 = addrspacecast i32* %ptr to i32 addrspace(1)*
+  br label %block2
+
+block2:
+  %z2 = addrspacecast i32* %ptr to i32 addrspace(1)*
+  store i32 addrspace(1)* %z1, i32 addrspace(1)** undef
+  ret i32 addrspace(1)* %z2
+}
+
+define i32 addrspace(1)* @addrspacecast_simplify(i32 addrspace(1)* %ptr) {
+; CHECK-LABEL: @addrspacecast_simplify(
+; CHECK-NEXT:  block1:
+; CHECK-NEXT:    [[CAST0:%.*]] = addrspacecast i32 addrspace(1)* [[PTR:%.*]] to i32*
+; CHECK-NEXT:    br label [[BLOCK2:%.*]]
+; CHECK:       block2:
+; CHECK-NEXT:    store i32 addrspace(1)* [[PTR]], i32 addrspace(1)** undef
+; CHECK-NEXT:    ret i32 addrspace(1)* [[PTR]]
+;
+block1:
+  %cast0 = addrspacecast i32 addrspace(1)* %ptr to i32*
+  %z1 = addrspacecast i32* %cast0 to i32 addrspace(1)*
+  br label %block2
+
+block2:
+  %z2 = addrspacecast i32* %cast0 to i32 addrspace(1)*
+  store i32 addrspace(1)* %z1, i32 addrspace(1)** undef
+  ret i32 addrspace(1)* %z2
+}
+
+@h = common local_unnamed_addr global i32* null, align 4
+
+define i32 addrspace(1)* @addrspacecast_constant() {
+; CHECK-LABEL: @addrspacecast_constant(
+; CHECK-NEXT:  block1:
+; CHECK-NEXT:    store i32* undef, i32** @h, align 4
+; CHECK-NEXT:    br label [[BLOCK2:%.*]]
+; CHECK:       block2:
+; CHECK-NEXT:    store i32 addrspace(1)* undef, i32 addrspace(1)** undef
+; CHECK-NEXT:    ret i32 addrspace(1)* undef
+;
+block1:
+  store i32* undef, i32** @h, align 4
+  %ptr = load i32*, i32** @h, align 4
+  %z1 = addrspacecast i32 * %ptr to i32 addrspace(1)*
+  br label %block2
+
+block2:
+  %z2 = addrspacecast i32* %ptr to i32 addrspace(1)*
+  store i32 addrspace(1)* %z1, i32 addrspace(1)** undef
+  ret i32 addrspace(1)* %z2
+}