[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 ↗

(On Diff #353573)

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

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());
+      Type *SrcElemTy =
+          !SrcPtrTy->isOpaque() ? SrcPtrTy->getElementType() : nullptr;
+      Type *DstElemTy =
+          !DstPtrTy->isOpaque() ? DstPtrTy->getElementType() : nullptr;
+      if (SrcElemTy == DstElemTy)
         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());
+  Type *DestElemTy = !DestTy->isOpaque() ? DestTy->getElementType() : nullptr;
+  Type *SrcElemTy = !SrcTy->isOpaque() ? SrcTy->getElementType() : nullptr;
+  if (SrcElemTy != DestElemTy) {
+    Type *MidTy = DestElemTy
+        ? PointerType::get(DestElemTy, SrcTy->getAddressSpace())
+        : PointerType::get(SrcTy->getContext(), 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

 ; CHECK-LABEL: @bitcast_opaque_to_typed(
 ; CHECK-NEXT:    [[B:%.*]] = bitcast ptr [[A:%.*]] to i8*
 ; CHECK-NEXT:    ret i8* [[B]]
 ;
   %b = bitcast ptr %a to i8*
   ret i8* %b
 }
 
-;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
+}