[InferAddressSpaces] Prevent crash on nested ConstantExpr

[joey]

Summary

Seems like everything on PostOrderStack is expected to be of pointer type.

[arsenm]

lib/Transforms/Scalar/InferAddressSpaces.cpp

326–327

I think this is a broader problem with vectors of pointers. This should maybe be checking this is a scalar pointer instead?

test/Transforms/InferAddressSpaces/AMDGPU/infer-address-space.ll

171

I think this is really a vector of pointers bug

[joey]

Sorry i should have provided more context, this is the original crash:

opt: ../lib/Transforms/Scalar/InferAddressSpaces.cpp:316: void {anonymous}::InferAddressSpaces::appendsFlatAddressExpressionToPostorderStack(llvm::Value*, std::vector<std::pair<llvm::Value*, bool> >&, llvm::DenseSet<llvm::Value*>&) const: Assertion `V->getType()->isPointerTy()' failed.

I changed the test to use a scalar pointer and still got the same crash.

appendsFlatAddressExpressionToPostorderStack gets called with %add.ptr157 as V. Then the it iterates over the operands, and it sees the select and isAddressExpression returns true.

After the select is added to PostorderStack the loop inside collectFlatAddressExpressions calls getPointerOperands. In my test case, this returns i64 73 and then crashes since this is not a pointer.

Maybe isAddressExpression should also check if the result of the select is a pointer?

[arsenm]

I misread the test case, this isn't a vector of pointers. If you can simplify the test by removing the vectors, that would be slightly better.

lib/Transforms/Scalar/InferAddressSpaces.cpp

326–327

I think this might be the wrong place. isAddressExpression should probably be doing the type check for the select case, since the other operations imply pointers (except PHI. I imagine you can craft another test case that fails with them)

[arsenm]

lib/Transforms/Scalar/InferAddressSpaces.cpp

326–327

This will only happen for constant expressions, so phi isn't an issue. Can you move the type check into the case for select, and assert the phi is a pointer type in isAddressExpression?

[joey]

Add the changes to isAddExpression instead.

[arsenm]

LGTM

[joey]

Committed revision r354576.

lib/Transforms/Scalar/InferAddressSpaces.cpp

 
 // Returns true if V is an address expression.
 // TODO: Currently, we consider only phi, bitcast, addrspacecast, and
 // getelementptr operators.
 static bool isAddressExpression(const Value &V) {
   if (!isa<Operator>(V))
     return false;
 
-  switch (cast<Operator>(V).getOpcode()) {
+  const Operator &Op = cast<Operator>(V);
+  switch (Op.getOpcode()) {
   case Instruction::PHI:
+    assert(Op.getType()->isPointerTy());
   case Instruction::BitCast:
   case Instruction::AddrSpaceCast:
   case Instruction::GetElementPtr:
-  case Instruction::Select:
     return true;
+  case Instruction::Select:
+    return Op.getType()->isPointerTy();
   default:
     return false;
   }
 }
 
 // Returns the pointer operands of V.
 //
 // Precondition: V is an address expression.
void InferAddressSpaces::appendsFlatAddressExpressionToPostorderStack(
   if (ConstantExpr *CE = dyn_cast<ConstantExpr>(V)) {
     // TODO: Look in non-address parts, like icmp operands.
     if (isAddressExpression(*CE) && Visited.insert(CE).second)
       PostorderStack.push_back(std::make_pair(CE, false));
 
     return;
   }
 
   if (isAddressExpression(*V) &&
       V->getType()->getPointerAddressSpace() == FlatAddrSpace) {
     if (Visited.insert(V).second) {
       PostorderStack.push_back(std::make_pair(V, false));
 
       Operator *Op = cast<Operator>(V);
       for (unsigned I = 0, E = Op->getNumOperands(); I != E; ++I) {
         if (ConstantExpr *CE = dyn_cast<ConstantExpr>(Op->getOperand(I))) {
           if (isAddressExpression(*CE) && Visited.insert(CE).second)
             PostorderStack.emplace_back(CE, false);

test/Transforms/InferAddressSpaces/AMDGPU/infer-address-space.ll

loop:                                             ; preds = %loop, %entry
   %i2 = getelementptr float, float* %i, i64 1
   %exit_cond = icmp eq float* %i2, %end
   br i1 %exit_cond, label %exit, label %loop
 
 exit:                                             ; preds = %loop
   ret void
 }
 
+; CHECK-LABEL: @select_bug(
+; CHECK: %add.ptr157 = getelementptr inbounds i64, i64* undef, i64 select (i1 icmp ne (i32* inttoptr (i64 4873 to i32*), i32* null), i64 73, i64 93)
+; CHECK: %cmp169 = icmp uge i64* undef, %add.ptr157
+define void @select_bug() #0 {
+  %add.ptr157 = getelementptr inbounds i64, i64* undef, i64 select (i1 icmp ne (i32* inttoptr (i64 4873 to i32*), i32* null), i64 73, i64 93)
+  %cmp169 = icmp uge i64* undef, %add.ptr157
+  unreachable
+}
+
 declare void @llvm.amdgcn.s.barrier() #1
 declare void @use(float) #0
 
 attributes #0 = { nounwind }
 attributes #1 = { convergent nounwind }