[OpaquePtr] Handle addrspacecasts in InstCombine

[nikic]

Summary

This adds support for addrspace casts involving opaque pointers to InstCombine, as well as the isEliminableCastPair() helper (otherwise the assertion failure would just move there).

Any thoughts on the approach here? Is the !isOpaque() ? getElementType() : nullptr pattern something we should add a helper function for? Not sure how common this is going to be outside these cast transforms.

[nikic]

Use getWithSamePointeeType().

[nikic]

Any thoughts on the approach here? Is the !isOpaque() ? getElementType() : nullptr pattern something we should add a helper function for? Not sure how common this is going to be outside these cast transforms.

Or possibly PointerType::hasSameElementTypeAs(PointerType *) would make sense here.

[aeubanks]

In D104668#2831687, @nikic wrote:

Any thoughts on the approach here? Is the !isOpaque() ? getElementType() : nullptr pattern something we should add a helper function for? Not sure how common this is going to be outside these cast transforms.

Or possibly PointerType::hasSameElementTypeAs(PointerType *) would make sense here.

I think PointerType::hasSameElementTypeAs(PointerType *) is good, we already have weird PointerType methods that are only for the opaque pointer transition, like PointerType::getWithSamePointeeType(), that aren't used in a lot of places and are to be deleted after.

[dblaikie]

In D104668#2831751, @aeubanks wrote:

In D104668#2831687, @nikic wrote:

Any thoughts on the approach here? Is the !isOpaque() ? getElementType() : nullptr pattern something we should add a helper function for? Not sure how common this is going to be outside these cast transforms.

Or possibly PointerType::hasSameElementTypeAs(PointerType *) would make sense here.

I think PointerType::hasSameElementTypeAs(PointerType *) is good, we already have weird PointerType methods that are only for the opaque pointer transition, like PointerType::getWithSamePointeeType(), that aren't used in a lot of places and are to be deleted after.

*thumbs up* I hope these sort of helpers will make the transition a bit easier - they can become no-ops/have their non-opaque parts removed at the appropriate point in the transition, then simple/clear/obvious change to remove the function and have callers do the right thing in a cleanup/NFC change.

[nikic]

Add PointerType::hasSameElementTypeAs().

[aeubanks]

lgtm with one nit

llvm/include/llvm/IR/DerivedTypes.h

701

could you add a note that this is purely for the opaque pointer transition

llvm/include/llvm/IR/DerivedTypes.h

public:
   /// Return true if either this is an opaque pointer type or if this pointee
   /// type matches Ty. Primarily used for checking if an instruction's pointer
   /// operands are valid types. Will be useless after non-opaque pointers are
   /// removed.
   bool isOpaqueOrPointeeTypeMatches(Type *Ty) {
     return isOpaque() || PointeeTy == Ty;
   }
 
+  /// Return true if both pointer types have the same element type. Two opaque
+  /// pointers are considered to have the same element type, while an opaque
+  /// and a non-opaque pointer have different element types.
+  /// TODO: Remove after opaque pointer transition is complete.
+  bool hasSameElementTypeAs(PointerType *Other) {
+    return PointeeTy == Other->PointeeTy;
+  }
+
   /// Implement support type inquiry through isa, cast, and dyn_cast.
   static bool classof(const Type *T) {
     return T->getTypeID() == PointerTyID;
   }
 };
 
 Type *Type::getExtendedType() const {
   assert(

llvm/lib/IR/Instructions.cpp

case 13:
         SrcTy->isPtrOrPtrVectorTy() &&
         MidTy->isPtrOrPtrVectorTy() &&
         DstTy->isPtrOrPtrVectorTy() &&
         SrcTy->getPointerAddressSpace() != MidTy->getPointerAddressSpace() &&
         MidTy->getPointerAddressSpace() == DstTy->getPointerAddressSpace() &&
         "Illegal addrspacecast, bitcast sequence!");
       // Allowed, use first cast's opcode
       return firstOp;
-    case 14:
+    case 14: {
       // bitcast, addrspacecast -> addrspacecast if the element type of
       // bitcast's source is the same as that of addrspacecast's destination.
-      if (SrcTy->getScalarType()->getPointerElementType() ==
-          DstTy->getScalarType()->getPointerElementType())
+      PointerType *SrcPtrTy = cast<PointerType>(SrcTy->getScalarType());
+      PointerType *DstPtrTy = cast<PointerType>(DstTy->getScalarType());
+      if (SrcPtrTy->hasSameElementTypeAs(DstPtrTy))
         return Instruction::AddrSpaceCast;
       return 0;
+    }
     case 15:
       // FIXME: this state can be merged with (1), but the following assert
       // is useful to check the correcteness of the sequence due to semantic
       // change of bitcast.
       assert(
         SrcTy->isIntOrIntVectorTy() &&
         MidTy->isPtrOrPtrVectorTy() &&
         DstTy->isPtrOrPtrVectorTy() &&

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

 Instruction *InstCombinerImpl::visitAddrSpaceCast(AddrSpaceCastInst &CI) {
   // If the destination pointer element type is not the same as the source's
   // first do a bitcast to the destination type, and then the addrspacecast.
   // This allows the cast to be exposed to other transforms.
   Value *Src = CI.getOperand(0);
   PointerType *SrcTy = cast<PointerType>(Src->getType()->getScalarType());
   PointerType *DestTy = cast<PointerType>(CI.getType()->getScalarType());
 
-  Type *DestElemTy = DestTy->getElementType();
-  if (SrcTy->getElementType() != DestElemTy) {
-    Type *MidTy = PointerType::get(DestElemTy, SrcTy->getAddressSpace());
+  if (!SrcTy->hasSameElementTypeAs(DestTy)) {
+    Type *MidTy =
+        PointerType::getWithSamePointeeType(DestTy, SrcTy->getAddressSpace());
     // Handle vectors of pointers.
     if (VectorType *VT = dyn_cast<VectorType>(CI.getType()))
       MidTy = VectorType::get(MidTy, VT->getElementCount());
 
     Value *NewBitCast = Builder.CreateBitCast(Src, MidTy);
     return new AddrSpaceCastInst(NewBitCast, CI.getType());
   }
 
   return commonPointerCastTransforms(CI);
 }

llvm/test/Transforms/InstCombine/opaque-ptr.ll

 @g = global i8 0
 define ptr @bitcast_typed_to_opaque_constexpr() {
 ; CHECK-LABEL: @bitcast_typed_to_opaque_constexpr(
 ; CHECK-NEXT:    ret ptr bitcast (i8* @g to ptr)
 ;
   ret ptr bitcast (i8* @g to ptr)
 }
 
-;define ptr @addrspacecast_opaque_to_opaque(ptr addrspace(1) %a) {
-;  %b = addrspacecast ptr addrspace(1) %a to ptr
-;  ret ptr %b
-;}
-
-;define ptr @addrspacecast_typed_to_opaque(i8 addrspace(1)* %a) {
-;  %b = addrspacecast i8 addrspace(1)* %a to ptr
-;  ret ptr %b
-;}
-
-;define i8* @addrspacecast_opaque_to_typed(ptr addrspace(1) %a) {
-;  %b = addrspacecast ptr addrspace(1) %a to i8*
-;  ret i8* %b
-;}
+define ptr @addrspacecast_opaque_to_opaque(ptr addrspace(1) %a) {
+; CHECK-LABEL: @addrspacecast_opaque_to_opaque(
+; CHECK-NEXT:    [[B:%.*]] = addrspacecast ptr addrspace(1) [[A:%.*]] to ptr
+; CHECK-NEXT:    ret ptr [[B]]
+;
+  %b = addrspacecast ptr addrspace(1) %a to ptr
+  ret ptr %b
+}
+
+define ptr @addrspacecast_typed_to_opaque(i8 addrspace(1)* %a) {
+; CHECK-LABEL: @addrspacecast_typed_to_opaque(
+; CHECK-NEXT:    [[TMP1:%.*]] = bitcast i8 addrspace(1)* [[A:%.*]] to ptr addrspace(1)
+; CHECK-NEXT:    [[B:%.*]] = addrspacecast ptr addrspace(1) [[TMP1]] to ptr
+; CHECK-NEXT:    ret ptr [[B]]
+;
+  %b = addrspacecast i8 addrspace(1)* %a to ptr
+  ret ptr %b
+}
+
+define i8* @addrspacecast_opaque_to_typed(ptr addrspace(1) %a) {
+; CHECK-LABEL: @addrspacecast_opaque_to_typed(
+; CHECK-NEXT:    [[TMP1:%.*]] = bitcast ptr addrspace(1) [[A:%.*]] to i8 addrspace(1)*
+; CHECK-NEXT:    [[B:%.*]] = addrspacecast i8 addrspace(1)* [[TMP1]] to i8*
+; CHECK-NEXT:    ret i8* [[B]]
+;
+  %b = addrspacecast ptr addrspace(1) %a to i8*
+  ret i8* %b
+}
+
+define ptr addrspace(1) @bitcast_and_addrspacecast_eliminable(ptr %a) {
+; CHECK-LABEL: @bitcast_and_addrspacecast_eliminable(
+; CHECK-NEXT:    [[C:%.*]] = addrspacecast ptr [[A:%.*]] to ptr addrspace(1)
+; CHECK-NEXT:    ret ptr addrspace(1) [[C]]
+;
+  %b = bitcast ptr %a to i8*
+  %c = addrspacecast i8* %b to ptr addrspace(1)
+  ret ptr addrspace(1) %c
+}
 
 define ptr @gep_constexpr_1(ptr %a) {
 ; CHECK-LABEL: @gep_constexpr_1(
 ; CHECK-NEXT:    ret ptr inttoptr (i64 6 to ptr)
 ;
   ret ptr getelementptr (i16, ptr null, i32 3)
 }
 
 define ptr @gep_constexpr_2(ptr %a) {
 ; CHECK-LABEL: @gep_constexpr_2(
 ; CHECK-NEXT:    ret ptr bitcast (i8* getelementptr (i8, i8* @g, i64 3) to ptr)
 ;
   ret ptr getelementptr (i8, ptr bitcast (i8* @g to ptr), i32 3)
 }