[Lint] Avoid failed assertion by fetching the proper pointer type

[uabelho]

Summary

When checking if a constant expression is a noop cast we fetched the
IntPtrType by doing DL->getIntPtrType(V->getType())). However, there can
be cases where V doesn't return a pointer, and then getIntPtrType()
triggers an assertion.

Now we pass DataLayout to isNoopCast so the method itself can determine
what the IntPtrType is.

[uabelho]

Picked you as reviewer Matt since you added the CastInst::isNoopCast(const DataLayout *DL) method
which I stole the fix from.

I've run into two different cases where Lint crashes due to the problem this patch tries to solve,
added them both in the new testcase.

[arsenm]

lib/Analysis/Lint.cpp

684–693

This is a convoluted way to go about this. This looks like some weird API artifact from before the DataLayout was mandatory. We should change isNoopCast to take the DataLayout instead (it looks like the instruction method version already does this)

[uabelho]

Pass DataLayout to isNoopCast when called from lint.

Question:
In FastISel::hasTrivialKill there is:

// No-op casts are trivially coalesced by fast-isel.
if (const auto *Cast = dyn_cast<CastInst>(I))
  if (Cast->isNoopCast(DL.getIntPtrType(Cast->getContext())) &&

Do you know if it is ok to change this isNoopCast call to Cast->isNoopCast(DL)?

If so, then I can clean up the patch further by removing the IntPtrTy versions completely.

[uabelho]

Ping

[arsenm]

In D37894#873521, @uabelho wrote:

Pass DataLayout to isNoopCast when called from lint.

Question:
In FastISel::hasTrivialKill there is:

// No-op casts are trivially coalesced by fast-isel.
if (const auto *Cast = dyn_cast<CastInst>(I))
  if (Cast->isNoopCast(DL.getIntPtrType(Cast->getContext())) &&

Do you know if it is ok to change this isNoopCast call to Cast->isNoopCast(DL)?

If so, then I can clean up the patch further by removing the IntPtrTy versions completely.

Yes, that should be fine. That can be a separate patch

[arsenm]

LGTM

[uabelho]

In D37894#886215, @arsenm wrote:

In D37894#873521, @uabelho wrote:

Pass DataLayout to isNoopCast when called from lint.

Question:
In FastISel::hasTrivialKill there is:

// No-op casts are trivially coalesced by fast-isel.
if (const auto *Cast = dyn_cast<CastInst>(I))
  if (Cast->isNoopCast(DL.getIntPtrType(Cast->getContext())) &&

Do you know if it is ok to change this isNoopCast call to Cast->isNoopCast(DL)?

If so, then I can clean up the patch further by removing the IntPtrTy versions completely.

Yes, that should be fine. That can be a separate patch

Alright I'll do that.
Thanks!

[uabelho]

If so, then I can clean up the patch further by removing the IntPtrTy versions completely.

Yes, that should be fine. That can be a separate patch

Alright I'll do that.
Thanks!

Here we go:
https://reviews.llvm.org/D38497

include/llvm/IR/InstrTypes.h

public:
   /// @brief Determine if the described cast is a no-op cast.
   static bool isNoopCast(
     Instruction::CastOps Opcode,  ///< Opcode of cast
     Type *SrcTy,   ///< SrcTy of cast
     Type *DstTy,   ///< DstTy of cast
     Type *IntPtrTy ///< Integer type corresponding to Ptr types
   );
 
+  /// @brief Determine if the described cast is a no-op cast.
+  static bool isNoopCast(
+    Instruction::CastOps Opcode, ///< Opcode of cast
+    Type *SrcTy,         ///< SrcTy of cast
+    Type *DstTy,         ///< DstTy of cast
+    const DataLayout &DL ///< DataLayout to get the Int Ptr type from.
+  );
+
   /// @brief Determine if this cast is a no-op cast.
   bool isNoopCast(
     Type *IntPtrTy ///< Integer type corresponding to pointer
   ) const;
 
   /// @brief Determine if this cast is a no-op cast.
   ///
   /// \param DL is the DataLayout to get the Int Ptr type from.

lib/Analysis/Lint.cpp

Value *Lint::findValueImpl(Value *V, bool OffsetOk,
   } else if (ExtractValueInst *Ex = dyn_cast<ExtractValueInst>(V)) {
     if (Value *W = FindInsertedValue(Ex->getAggregateOperand(),
                                      Ex->getIndices()))
       if (W != V)
         return findValueImpl(W, OffsetOk, Visited);
   } else if (ConstantExpr *CE = dyn_cast<ConstantExpr>(V)) {
     // Same as above, but for ConstantExpr instead of Instruction.
     if (Instruction::isCast(CE->getOpcode())) {
       if (CastInst::isNoopCast(Instruction::CastOps(CE->getOpcode()),
                                CE->getOperand(0)->getType(), CE->getType(),
-                               DL->getIntPtrType(V->getType())))
+                               *DL))
         return findValueImpl(CE->getOperand(0), OffsetOk, Visited);
     } else if (CE->getOpcode() == Instruction::ExtractValue) {
       ArrayRef<unsigned> Indices = CE->getIndices();
       if (Value *W = FindInsertedValue(CE->getOperand(0), Indices))
         if (W != V)
           return findValueImpl(W, OffsetOk, Visited);
     }
   }
 
   // As a last resort, try SimplifyInstruction or constant folding.
   if (Instruction *Inst = dyn_cast<Instruction>(V)) {
     if (Value *W = SimplifyInstruction(Inst, {*DL, TLI, DT, AC}))
       return findValueImpl(W, OffsetOk, Visited);
   } else if (auto *C = dyn_cast<Constant>(V)) {
     if (Value *W = ConstantFoldConstant(C, *DL, TLI))

lib/IR/Instructions.cpp

case Instruction::PtrToInt:
              DestTy->getScalarSizeInBits();
     case Instruction::IntToPtr:
       return IntPtrTy->getScalarSizeInBits() ==
              SrcTy->getScalarSizeInBits();
   }
 }
 
 /// @brief Determine if a cast is a no-op.
+bool CastInst::isNoopCast(Instruction::CastOps Opcode,
+                          Type *SrcTy,
+                          Type *DestTy,
+                          const DataLayout &DL) {
+  Type *PtrOpTy = nullptr;
+  if (Opcode == Instruction::PtrToInt)
+    PtrOpTy = SrcTy;
+  else if (Opcode == Instruction::IntToPtr)
+    PtrOpTy = DestTy;
+
+  Type *IntPtrTy = PtrOpTy ? DL.getIntPtrType(PtrOpTy) :
+                             DL.getIntPtrType(SrcTy->getContext(), 0);
+
+  return isNoopCast(Opcode, SrcTy, DestTy, IntPtrTy);
+}
+
+/// @brief Determine if a cast is a no-op.
 bool CastInst::isNoopCast(Type *IntPtrTy) const {
   return isNoopCast(getOpcode(), getOperand(0)->getType(), getType(), IntPtrTy);
 }
 
 bool CastInst::isNoopCast(const DataLayout &DL) const {
-  Type *PtrOpTy = nullptr;
-  if (getOpcode() == Instruction::PtrToInt)
-    PtrOpTy = getOperand(0)->getType();
-  else if (getOpcode() == Instruction::IntToPtr)
-    PtrOpTy = getType();
-
-  Type *IntPtrTy =
-      PtrOpTy ? DL.getIntPtrType(PtrOpTy) : DL.getIntPtrType(getContext(), 0);
-
-  return isNoopCast(getOpcode(), getOperand(0)->getType(), getType(), IntPtrTy);
+  return isNoopCast(getOpcode(), getOperand(0)->getType(), getType(), DL);
 }
 
 /// This function determines if a pair of casts can be eliminated and what
 /// opcode should be used in the elimination. This assumes that there are two
 /// instructions like this:
 /// *  %F = firstOpcode SrcTy %x to MidTy
 /// *  %S = secondOpcode MidTy %F to DstTy
 /// The function returns a resultOpcode so these two casts can be replaced with:

test/Analysis/Lint/noop-cast-expr-no-pointer.ll

+; RUN: opt -lint < %s
+
+; lint shouldn't crash on any of the below functions
+
+@g_1 = external global [3 x i32]
+@g_2 = external global [2 x i32]
+
+define void @test1() {
+entry:
+  tail call void @f1(i16 zext (i1 icmp eq (i32* getelementptr inbounds ([2 x i32], [2 x i32]* @g_2, i64 0, i64 0), i32* getelementptr inbounds ([3 x i32], [3 x i32]* @g_1, i64 0, i64 1)) to i16))
+  ret void
+}
+
+declare void @f1(i16)
+
+define void @test2() {
+  tail call void inttoptr (i64 sext (i32 ptrtoint (void ()* @f2 to i32) to i64) to void ()*)()
+
+  ret void
+}
+
+declare void @f2()
+