Skip to content

[clang] Replace uses of CreatePointerBitCastOrAddrSpaceCast (NFC) #68277

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Nov 11, 2023
Merged

[clang] Replace uses of CreatePointerBitCastOrAddrSpaceCast (NFC) #68277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2c3fb03
[clang] Replace uses of CreatePointerBitCastOrAddrSpaceCast (NFC)
JOE1994 Oct 2, 2023
578bc43
Update comment for CreatePointerBitCastOrAddrSpaceCast
JOE1994 Oct 6, 2023
c5d41cd
git clang-format
JOE1994 Oct 6, 2023
936247e
Update type of 'result.ptr'
JOE1994 Nov 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clang/lib/CodeGen/CGCall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
8 changes: 3 additions & 5 deletions clang/lib/CodeGen/CodeGenFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,11 +1121,9 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
Address(&*AI, ConvertType(RetTy),
CurFnInfo->getReturnInfo().getIndirectAlign(), KnownNonNull);
if (!CurFnInfo->getReturnInfo().getIndirectByVal()) {
ReturnValuePointer =
CreateDefaultAlignTempAlloca(Int8PtrTy, "result.ptr");
Builder.CreateStore(Builder.CreatePointerBitCastOrAddrSpaceCast(
ReturnValue.getPointer(), Int8PtrTy),
ReturnValuePointer);
ReturnValuePointer = CreateDefaultAlignTempAlloca(
ReturnValue.getPointer()->getType(), "result.ptr");
Builder.CreateStore(ReturnValue.getPointer(), ReturnValuePointer);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
} else if (CurFnInfo->getReturnInfo().getKind() == ABIArgInfo::InAlloca &&
!hasScalarEvaluationKind(CurFnInfo->getReturnType())) {
Expand Down
3 changes: 0 additions & 3 deletions clang/lib/CodeGen/ItaniumCXXABI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Val, Wrapper->getReturnType(), "");

Builder.CreateRet(Val);
}
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/CodeGen/TargetInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" : "");
}

Expand Down
3 changes: 3 additions & 0 deletions llvm/include/llvm/IR/IRBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2138,6 +2138,9 @@ class IRBuilderBase {
return Insert(CastInst::CreatePointerCast(V, DestTy), Name);
}

// With opaque pointers enabled, this can be substituted with
// CreateAddrSpaceCast.
// TODO: Replace uses of this method and remove the method itself.
Value *CreatePointerBitCastOrAddrSpaceCast(Value *V, Type *DestTy,
const Twine &Name = "") {
if (V->getType() == DestTy)
Expand Down