[clang][CodeGen] Make `UnqualPtrTy` truly unqualified #94388

AlexVlx · 2024-06-04T18:54:58Z

At the moment we alias UnqualPtrTy with the other default AS pointer types, which means that for certain targets using it doesn't actually yield an unqualified pointer. This patch changes that so that it does exactly what it says on the tin, under the assumption that if someone asks for an unqualified pointer they do so knowingly and expect to get exactly that.

llvmbot · 2024-06-04T18:55:27Z

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Alex Voicu (AlexVlx)

Changes

At the moment we alias UnqualPtrTy with the other default AS pointer types, which means that for certain targets using it doesn't actually yield an unqualified pointer. This patch changes that so that it does exactly what it says on the tin, under the assumption that if someone asks for an unqualified pointer they do so knowingly and expect to get exactly that.

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

3 Files Affected:

(modified) clang/lib/CodeGen/CodeGenModule.cpp (+1)
(modified) clang/lib/CodeGen/CodeGenTypeCache.h (+3-1)
(modified) clang/lib/CodeGen/ItaniumCXXABI.cpp (+3-3)

diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index be7bf0b72dc0c..b58ca03ce69da 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -368,6 +368,7 @@ CodeGenModule::CodeGenModule(ASTContext &C,
   IntTy = llvm::IntegerType::get(LLVMContext, C.getTargetInfo().getIntWidth());
   IntPtrTy = llvm::IntegerType::get(LLVMContext,
     C.getTargetInfo().getMaxPointerWidth());
+  UnqualPtrTy = llvm::PointerType::getUnqual(LLVMContext);
   Int8PtrTy = llvm::PointerType::get(LLVMContext,
                                      C.getTargetAddressSpace(LangAS::Default));
   const llvm::DataLayout &DL = M.getDataLayout();
diff --git a/clang/lib/CodeGen/CodeGenTypeCache.h b/clang/lib/CodeGen/CodeGenTypeCache.h
index e273ebe3b060f..3b659bc182aa4 100644
--- a/clang/lib/CodeGen/CodeGenTypeCache.h
+++ b/clang/lib/CodeGen/CodeGenTypeCache.h
@@ -51,9 +51,11 @@ struct CodeGenTypeCache {
     llvm::IntegerType *PtrDiffTy;
   };
 
+  /// unqualified void*
+  llvm::PointerType *UnqualPtrTy;
+
   /// void*, void** in the target's default address space (often 0)
   union {
-    llvm::PointerType *UnqualPtrTy;
     llvm::PointerType *VoidPtrTy;
     llvm::PointerType *Int8PtrTy;
     llvm::PointerType *VoidPtrPtrTy;
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 8427286dee887..8952e7ca072da 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -4656,14 +4656,14 @@ static void InitCatchParam(CodeGenFunction &CGF,
   auto catchRD = CatchType->getAsCXXRecordDecl();
   CharUnits caughtExnAlignment = CGF.CGM.getClassPointerAlignment(catchRD);
 
-  llvm::Type *PtrTy = CGF.UnqualPtrTy; // addrspace 0 ok
+  llvm::Type *PtrTy = CGF.UnqualPtrTy; // unqualified is ok
 
   // Check for a copy expression.  If we don't have a copy expression,
   // that means a trivial copy is okay.
   const Expr *copyExpr = CatchParam.getInit();
   if (!copyExpr) {
     llvm::Value *rawAdjustedExn = CallBeginCatch(CGF, Exn, true);
-    Address adjustedExn(CGF.Builder.CreateBitCast(rawAdjustedExn, PtrTy),
+    Address adjustedExn(CGF.Builder.CreatePointerCast(rawAdjustedExn, PtrTy),
                         LLVMCatchTy, caughtExnAlignment);
     LValue Dest = CGF.MakeAddrLValue(ParamAddr, CatchType);
     LValue Src = CGF.MakeAddrLValue(adjustedExn, CatchType);
@@ -4677,7 +4677,7 @@ static void InitCatchParam(CodeGenFunction &CGF,
     CGF.EmitNounwindRuntimeCall(getGetExceptionPtrFn(CGF.CGM), Exn);
 
   // Cast that to the appropriate type.
-  Address adjustedExn(CGF.Builder.CreateBitCast(rawAdjustedExn, PtrTy),
+  Address adjustedExn(CGF.Builder.CreatePointerCast(rawAdjustedExn, PtrTy),
                       LLVMCatchTy, caughtExnAlignment);
 
   // The copy expression is defined in terms of an OpaqueValueExpr.

…al_ptr_ty

jrtc27 · 2024-06-04T18:59:35Z

I don't think that assumption is currently true. I think it's also worth clarifying what this thing is, and possibly renaming it, because "unqualified" has C language level meaning that would contradict what it is here.

efriedma-quic · 2024-06-05T04:41:47Z

llvm::PointerType::getUnqual assumes pointers to addr-space 0 are "unqualified"... but the discussion on #88182 concluded that doesn't make sense: the "unqualified" address space is a target-specific choice, and address-space zero isn't required to have any meaning at all.

Given that, it seems like this is going in the opposite direction of what you want.

jrtc27 · 2024-06-05T04:50:55Z

I think the point is there are two things going on here that people think of as unqualified:

"Give me a pointer that's whatever void * is"
"Give me a pointer in some address space just so I can put a global in some metadata"

The vast majority of the time we're talking about 1, which is what most people would think of unqualified as meaning. #93601 / #93914 are about the three special cases that are case 2 above. But those are a special case, and normally when people say unqualified they mean "default" address space / void *.

AlexVlx · 2024-06-05T14:18:13Z

I think the point is there are two things going on here that people think of as unqualified:

"Give me a pointer that's whatever void * is"

"Give me a pointer in some address space just so I can put a global in some metadata"

The vast majority of the time we're talking about 1, which is what most people would think of unqualified as meaning. #93601 / #93914 are about the three special cases that are case 2 above. But those are a special case, and normally when people say unqualified they mean "default" address space / void *.

Building on the above, let me add a bit more colour I should have added from the get go:

More often than not, for case (1) above, folks probably want (knowingly or not) a void* that "works",
with no actual concern for the address space (the "default");
As an example, for targets which model themselves / draw inspiration from e.g. OpenCL, this'd be a
generic / flat pointer, but there's no way for the FE to query that from the target (today?), and that
information isn't really observably encoded in something like the DataLayout; if we were to add
something like that, then unqualified would / should correspond to this, IMHO (could well happen to be 0
in practice);
We have targets that map LangAS::Defaultoddly / differently from the above expectation, and hence it
appeared profitable to have UnqualPtrTy, which is a generally useful / everywhere usable pointer type,
be different from Int8PtrTy / VoidPtrTy which are pointer types which correspond to the target's
default (perhaps I am being overly cautious here though).

Having said that, this change is somewhat inert at the moment beyond carving out the name / making it shorthand for calling getUnqual(). In the future, we could either:

extend DL to expose a target's flat/generic AS
(more intrusive / might be infeasible) make unqualified ptrs be generic/everywhere usable in IR, without
the target exposing a flat/generic AS, letting the target figure out the legality of e.g. this or that ascast.

AlexVlx

I don't think that assumption is currently true. I think it's also worth clarifying what this thing is, and possibly renaming it, because "unqualified" has C language level meaning that would contradict what it is here.

It's probably optimistic, based on us also having Int8PtrTy / VoidPtrTy which seem less "scary" (for lack of a better word) and more familiar for someone coming from e.g. C. In what regards renaming, I don't have an immediate idea of what to rename it to, and it's tied to PointerType::getUnqual, so if we rename this we'd probably have to rename that as well, no?

…al_ptr_ty

arichardson · 2024-06-05T17:45:19Z

clang/lib/CodeGen/ItaniumCXXABI.cpp

@@ -4656,14 +4656,14 @@ static void InitCatchParam(CodeGenFunction &CGF,
  auto catchRD = CatchType->getAsCXXRecordDecl();
  CharUnits caughtExnAlignment = CGF.CGM.getClassPointerAlignment(catchRD);

-  llvm::Type *PtrTy = CGF.UnqualPtrTy; // addrspace 0 ok


I am not convinced this is actually correct, it's been a long time since I looked at C++ exception handling, but I would have assumed this to either be a stack variable or a global?

This should be a pointer to a heap-allocated object.

In any case, we shouldn't need to cast the return value of __cxa_begin_catch to a different address-space; we can just drop the bitcasts.

arichardson · 2024-06-05T17:45:35Z

clang/lib/CodeGen/CodeGenTypeCache.h

@@ -51,9 +51,11 @@ struct CodeGenTypeCache {
    llvm::IntegerType *PtrDiffTy;
  };

+  /// unqualified void*
+  llvm::PointerType *UnqualPtrTy;


I'd argue we should just remove this variable, and the few cases that actually want AS0 can call PointerType::get(0) directly.

I'm not opposed to this in principle; might have some additional benefits in that it forces people to knowingly opt for something via PointerType::getUnqual / PointerType::get(some_as), rather than accidentally stumbling into it.

…al_ptr_ty

arichardson

This looks fine to me now, but I'd suggest a TODO comment that UnqualPtrTy should be removed in the future.

jrtc27 · 2024-06-08T00:01:15Z

There are still quite a few references to UnqualPtrTy that you're not changing yet are now wrong, no?

$ git grep --count '\<UnqualPtrTy\>' origin/main clang
origin/main:clang/lib/CodeGen/CGAtomic.cpp:1
origin/main:clang/lib/CodeGen/CGBlocks.cpp:1
origin/main:clang/lib/CodeGen/CGBuiltin.cpp:13
origin/main:clang/lib/CodeGen/CGCUDANV.cpp:1
origin/main:clang/lib/CodeGen/CGExpr.cpp:2
origin/main:clang/lib/CodeGen/CGObjCMac.cpp:1
origin/main:clang/lib/CodeGen/CodeGenTypeCache.h:1
origin/main:clang/lib/CodeGen/ItaniumCXXABI.cpp:10
origin/main:clang/lib/CodeGen/Targets/PPC.cpp:1
origin/main:clang/lib/CodeGen/Targets/Sparc.cpp:1

jrtc27 · 2024-06-08T00:02:19Z

And I still strongly urge renaming what this is, given it is not the type for an unqualified C void *, as one would normally expect given it's in Clang. Perhaps DummyPtrTy?

arichardson

Ah I did not realize there were so many uses remaining. I that case those probably need auditing/changing before this change can land.

AlexVlx · 2024-06-08T00:25:59Z

And I still strongly urge renaming what this is, given it is not the type for an unqualified C void *, as one would normally expect given it's in Clang. Perhaps DummyPtrTy?

DummyPtrTy seems mnemonic / on point, but it might trigger antibodies from the LLVM side:) Perhaps NoAddrSpacePtrTy or something to that extent? What makes this unqualified is it being just a ptr and not a ptr addrspace(n). On the other hand, since today it's an unconditional alias for ptr addrspace(0), perhaps AS0PtrTy or somesuch thing could also work?

AlexVlx · 2024-06-08T00:27:09Z

There are still quite a few references to UnqualPtrTy that you're not changing yet are now wrong, no?

$ git grep --count '\<UnqualPtrTy\>' origin/main clang
origin/main:clang/lib/CodeGen/CGAtomic.cpp:1
origin/main:clang/lib/CodeGen/CGBlocks.cpp:1
origin/main:clang/lib/CodeGen/CGBuiltin.cpp:13
origin/main:clang/lib/CodeGen/CGCUDANV.cpp:1
origin/main:clang/lib/CodeGen/CGExpr.cpp:2
origin/main:clang/lib/CodeGen/CGObjCMac.cpp:1
origin/main:clang/lib/CodeGen/CodeGenTypeCache.h:1
origin/main:clang/lib/CodeGen/ItaniumCXXABI.cpp:10
origin/main:clang/lib/CodeGen/Targets/PPC.cpp:1
origin/main:clang/lib/CodeGen/Targets/Sparc.cpp:1

Right, going through these at the moment, I got sidetracked.

Unqualified ptrs should be truly unqualified, and not AS0 ptrs.

cdf95a8

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Jun 4, 2024

AlexVlx removed the clang Clang issues not falling into any other category label Jun 4, 2024

AlexVlx requested review from arichardson, efriedma-quic and jrtc27 June 4, 2024 18:55

Merge branch 'main' of https://github.com/llvm/llvm-project into unqu…

d05b71c

…al_ptr_ty

llvmbot added the clang Clang issues not falling into any other category label Jun 4, 2024

AlexVlx commented Jun 5, 2024

View reviewed changes

Merge branch 'main' of https://github.com/llvm/llvm-project into unqu…

b62f653

…al_ptr_ty

arichardson reviewed Jun 5, 2024

View reviewed changes

AlexVlx added 3 commits June 6, 2024 15:16

Merge branch 'main' of https://github.com/llvm/llvm-project into unqu…

894b48b

…al_ptr_ty

Pointer to default should work.

5eef3a9

Merge branch 'main' of https://github.com/llvm/llvm-project into unqu…

86c0ff9

…al_ptr_ty

arichardson approved these changes Jun 7, 2024

View reviewed changes

arichardson requested changes Jun 8, 2024

View reviewed changes

[clang][CodeGen] Make UnqualPtrTy truly unqualified #94388

Are you sure you want to change the base?

[clang][CodeGen] Make UnqualPtrTy truly unqualified #94388

Uh oh!

Conversation

AlexVlx commented Jun 4, 2024

Uh oh!

llvmbot commented Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrtc27 commented Jun 4, 2024

Uh oh!

efriedma-quic commented Jun 5, 2024

Uh oh!

jrtc27 commented Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexVlx commented Jun 5, 2024

Uh oh!

AlexVlx left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arichardson Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

efriedma-quic Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

arichardson Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

AlexVlx Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

arichardson left a comment

Choose a reason for hiding this comment

Uh oh!

jrtc27 commented Jun 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrtc27 commented Jun 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arichardson left a comment

Choose a reason for hiding this comment

Uh oh!

AlexVlx commented Jun 8, 2024

Uh oh!

AlexVlx commented Jun 8, 2024

Uh oh!

Uh oh!

[clang][CodeGen] Make `UnqualPtrTy` truly unqualified #94388

[clang][CodeGen] Make `UnqualPtrTy` truly unqualified #94388

llvmbot commented Jun 4, 2024 •

edited

Loading

jrtc27 commented Jun 5, 2024 •

edited

Loading

AlexVlx left a comment •

edited

Loading

jrtc27 commented Jun 8, 2024 •

edited

Loading

jrtc27 commented Jun 8, 2024 •

edited

Loading