Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OpenCL] Replace a CreatePointerCast call; NFC #112676

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

svenvh
Copy link
Member

@svenvh svenvh commented Oct 17, 2024

With opaque pointers, the only purpose of the cast here is to cast between address spaces, similar to the 4-argument case below.

With opaque pointers, the only purpose of the cast here is to cast
between address spaces, similar to the 4-argument case below.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen labels Oct 17, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 17, 2024

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Sven van Haastregt (svenvh)

Changes

With opaque pointers, the only purpose of the cast here is to cast between address spaces, similar to the 4-argument case below.


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

1 Files Affected:

  • (modified) clang/lib/CodeGen/CGBuiltin.cpp (+2-2)
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index f6d7db2c204c12..b2af4295fe99fe 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -5641,10 +5641,10 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
       llvm::Type *ArgTys[] = {Arg0->getType(), I8PTy, Int32Ty, Int32Ty};
       llvm::FunctionType *FTy = llvm::FunctionType::get(
           Int32Ty, llvm::ArrayRef<llvm::Type *>(ArgTys), false);
-      Value *BCast = Builder.CreatePointerCast(Arg1, I8PTy);
+      Value *ACast = Builder.CreateAddrSpaceCast(Arg1, I8PTy);
       return RValue::get(
           EmitRuntimeCall(CGM.CreateRuntimeFunction(FTy, Name),
-                          {Arg0, BCast, PacketSize, PacketAlign}));
+                          {Arg0, ACast, PacketSize, PacketAlign}));
     } else {
       assert(4 == E->getNumArgs() &&
              "Illegal number of parameters to pipe function");

@svenvh svenvh requested a review from arsenm October 17, 2024 14:00
@JOE1994
Copy link
Member

JOE1994 commented Oct 17, 2024

IRBuilderBase:CreatePointerCast doesn't emit anything if the from and to targets types are same:

Value *CreatePointerCast(Value *V, Type *DestTy,
const Twine &Name = "") {
if (V->getType() == DestTy)
return V;
if (auto *VC = dyn_cast<Constant>(V))
return Insert(Folder.CreatePointerCast(VC, DestTy), Name);
return Insert(CastInst::CreatePointerCast(V, DestTy), Name);
}

while IRBuilderBase::CreateAddrSpaceCast emits addrspacecast regardless:

Value *CreateAddrSpaceCast(Value *V, Type *DestTy,
const Twine &Name = "") {
return CreateCast(Instruction::AddrSpaceCast, V, DestTy, Name);
}

LLVM LangRef says that from and to pointer types of addrspacecast needs to have different address spaces
(not sure if this is outdated info)

@JOE1994 JOE1994 requested review from arsenm and nikic October 17, 2024 17:01
@JOE1994
Copy link
Member

JOE1994 commented Oct 17, 2024

Oh.. CreateAddrSpaceCast calls CreateCast which also initially checks whether to and from types are identical before emitting anything.

Value *CreateCast(Instruction::CastOps Op, Value *V, Type *DestTy,
const Twine &Name = "") {
if (V->getType() == DestTy)
return V;
if (Value *Folded = Folder.FoldCast(Op, V, DestTy))
return Folded;
return Insert(CastInst::Create(Op, V, DestTy), Name);
}

@svenvh svenvh merged commit 5a09ce9 into llvm:main Oct 18, 2024
11 checks passed
@svenvh svenvh deleted the cgbuiltin-pipe-ptrcast branch October 18, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants