[clang] Replace uses of CreatePointerBitCastOrAddrSpaceCast (NFC)

[JOE1994]

With opaque pointers, CreatePointerBitCastOrAddrSpaceCast is same as CreateAddrSpaceCast. Replace or remove uses of CreatePointerBitCastOrAddrSpaceCast.

Opaque pointer cleanup effort.

[llvmbot]

@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-clang

Changes

With opaque pointers, CreatePointerBitCastOrAddrSpaceCast is same as CreateAddrSpaceCast. Replace or remove uses of CreatePointerBitCastOrAddrSpaceCast.

Opaque pointer cleanup effort.

---

Full diff: https://github.com/llvm/llvm-project/pull/68277.diff

5 Files Affected:

• (modified) clang/lib/CodeGen/CGCall.cpp (+1-1)
• (modified) clang/lib/CodeGen/CodeGenFunction.cpp (+1-2)
• (modified) clang/lib/CodeGen/ItaniumCXXABI.cpp (-3)
• (modified) clang/lib/CodeGen/TargetInfo.cpp (+1-1)
• (modified) llvm/include/llvm/IR/IRBuilder.h (+2)

diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 93e16575042c4da..b0d1fcd26a5f413 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -1376,7 +1376,7 @@ static void CreateCoercedStore(llvm::Value *Src,
   llvm::PointerType *DstPtrTy = llvm::dyn_cast<llvm::PointerType>(DstTy);
   if (SrcPtrTy && DstPtrTy &&
       SrcPtrTy->getAddressSpace() != DstPtrTy->getAddressSpace()) {
-    Src = CGF.Builder.CreatePointerBitCastOrAddrSpaceCast(Src, DstTy);
+    Src = CGF.Builder.CreateAddrSpaceCast(Src, DstTy);
     CGF.Builder.CreateStore(Src, Dst, DstIsVolatile);
     return;
   }
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 9b21f428b0af7f5..05f8c459a346583 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1123,8 +1123,7 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
     if (!CurFnInfo->getReturnInfo().getIndirectByVal()) {
       ReturnValuePointer =
           CreateDefaultAlignTempAlloca(Int8PtrTy, "result.ptr");
-      Builder.CreateStore(Builder.CreatePointerBitCastOrAddrSpaceCast(
-                              ReturnValue.getPointer(), Int8PtrTy),
+      Builder.CreateStore(ReturnValue.getPointer(),
                           ReturnValuePointer);
     }
   } else if (CurFnInfo->getReturnInfo().getKind() == ABIArgInfo::InAlloca &&
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 0c89871420bdd3d..f7ef9503aa61033 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -3088,9 +3088,6 @@ void ItaniumCXXABI::EmitThreadLocalInitFuncs(
       CharUnits Align = CGM.getContext().getDeclAlign(VD);
       Val = Builder.CreateAlignedLoad(Var->getValueType(), Val, Align);
     }
-    if (Val->getType() != Wrapper->getReturnType())
-      Val = Builder.CreatePointerBitCastOrAddrSpaceCast(
-          Val, Wrapper->getReturnType(), "");
 
     Builder.CreateRet(Val);
   }
diff --git a/clang/lib/CodeGen/TargetInfo.cpp b/clang/lib/CodeGen/TargetInfo.cpp
index 3d79f92137abc79..60224d458f6a262 100644
--- a/clang/lib/CodeGen/TargetInfo.cpp
+++ b/clang/lib/CodeGen/TargetInfo.cpp
@@ -137,7 +137,7 @@ llvm::Value *TargetCodeGenInfo::performAddrSpaceCast(
   if (auto *C = dyn_cast<llvm::Constant>(Src))
     return performAddrSpaceCast(CGF.CGM, C, SrcAddr, DestAddr, DestTy);
   // Try to preserve the source's name to make IR more readable.
-  return CGF.Builder.CreatePointerBitCastOrAddrSpaceCast(
+  return CGF.Builder.CreateAddrSpaceCast(
       Src, DestTy, Src->hasName() ? Src->getName() + ".ascast" : "");
 }
 
diff --git a/llvm/include/llvm/IR/IRBuilder.h b/llvm/include/llvm/IR/IRBuilder.h
index c9f243fdb12e404..a32ec9a66fedfa8 100644
--- a/llvm/include/llvm/IR/IRBuilder.h
+++ b/llvm/include/llvm/IR/IRBuilder.h
@@ -2138,6 +2138,8 @@ class IRBuilderBase {
     return Insert(CastInst::CreatePointerCast(V, DestTy), Name);
   }
 
+  // With opaque pointers enabled, this is same as CreateAddressSpaceCast.
+  // TODO: Replace uses of this method and remove the method itself.
   Value *CreatePointerBitCastOrAddrSpaceCast(Value *V, Type *DestTy,
                                              const Twine &Name = "") {
     if (V->getType() == DestTy)

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[JOE1994]

Stripping the call to CreatePointerBitCastOrAddrSpaceCast doesn't cause any new ninja check-clang failures.

But.. I'm not entirely sure whether stripping the call is justified.

[s-barannikov]

I couldn't break it with something like:

using int_as1 = int __attribute((address_space(1)));
using int_as2 = int __attribute((address_space(2)));

extern int_as1 foo();

//thread_local const int_as2 &r = foo(); // error
//thread_local const int_as1 &r = foo(); // error (?)
thread_local const int & __attribute((address_space(1))) r = foo(); // compiles (?)

The behavior is not intuitive to me, however.

[JOE1994]

Maybe I should play safe by just replacing the call to CreateAddrSpaceCast instead of stripping it entirely.

[s-barannikov]

I'm in favor of stripping it. In the end, codebase simplification is the whole point of opaque pointers.
This check was added in https://reviews.llvm.org/D5353, which has nothing to do with address spaces.
If it ever breaks, it should be easy to fix.

[JOE1994]

Applied git clang-format.

[JOE1994]

No new regressions with ninja check-clang

[s-barannikov]

LGTM with the suggestion applied.

[bjope]

So ReturnValue.getPointer() is guaranteed to be an unqualified pointer?

I'm just wondering why this hasn't been just a pointer bitcast in the past. But even if it would cast away an address space it seems a bit weird.
Not sure if it is worth an assert now when we bypass all the castIsValid checks etc that would have been done in the past.

(And maybe the "result.ptr" should be created with ReturnValue.getPointer().getType() rather than using Int8PtrTy. Although I consider that as out of scope for this patch.)

[JOE1994]

But even if it would cast away an address space it seems a bit weird.

Yes, I agree..

(And maybe the "result.ptr" should be created with ReturnValue.getPointer().getType() rather than using Int8PtrTy. Although I consider that as out of scope for this patch.)

I also agree.
Since, adding the change to this revision doesn't add new failures with ninja check-clang, I think it can be merged to this patch. I'll add the change to this revision.