[Clang][NFC] Rename UnqualPtrTy to DefaultPtrTy #163207
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-powerpc @llvm/pr-subscribers-clang-codegen Author: Juan Manuel Martinez Caamaño (jmmartinez) ChangesThe This patch makes the The only change that I saw is a type mismatch on the ItaniumCXXABI for SPIRV (which I guess is unlikely to ever happen, but nevertheless it should work). Full diff: https://github.com/llvm/llvm-project/pull/163207.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 8d019d4b2da25..af66cafb7880b 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -437,6 +437,7 @@ CodeGenModule::CodeGenModule(ASTContext &C,
C.getTargetInfo().getMaxPointerWidth());
Int8PtrTy = llvm::PointerType::get(LLVMContext,
C.getTargetAddressSpace(LangAS::Default));
+ UnqualPtrTy = llvm::PointerType::getUnqual(LLVMContext);
const llvm::DataLayout &DL = M.getDataLayout();
AllocaInt8PtrTy =
llvm::PointerType::get(LLVMContext, DL.getAllocaAddrSpace());
diff --git a/clang/lib/CodeGen/CodeGenTypeCache.h b/clang/lib/CodeGen/CodeGenTypeCache.h
index e273ebe3b060f..6fe7c669be9ab 100644
--- a/clang/lib/CodeGen/CodeGenTypeCache.h
+++ b/clang/lib/CodeGen/CodeGenTypeCache.h
@@ -51,15 +51,17 @@ struct CodeGenTypeCache {
llvm::IntegerType *PtrDiffTy;
};
- /// void*, void** in the target's default address space (often 0)
+ /// void*, void** in the target's default address space (often 0, but not
+ /// always)
union {
- llvm::PointerType *UnqualPtrTy;
llvm::PointerType *VoidPtrTy;
llvm::PointerType *Int8PtrTy;
llvm::PointerType *VoidPtrPtrTy;
llvm::PointerType *Int8PtrPtrTy;
};
+ llvm::PointerType *UnqualPtrTy;
+
/// void* in alloca address space
union {
llvm::PointerType *AllocaVoidPtrTy;
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 7dc2eaf1e9f75..68c52e5ab244b 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -5039,7 +5039,7 @@ 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.Int8PtrTy;
// Check for a copy expression. If we don't have a copy expression,
// that means a trivial copy is okay.
|
|
@llvm/pr-subscribers-clang Author: Juan Manuel Martinez Caamaño (jmmartinez) ChangesThe This patch makes the The only change that I saw is a type mismatch on the ItaniumCXXABI for SPIRV (which I guess is unlikely to ever happen, but nevertheless it should work). Full diff: https://github.com/llvm/llvm-project/pull/163207.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 8d019d4b2da25..af66cafb7880b 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -437,6 +437,7 @@ CodeGenModule::CodeGenModule(ASTContext &C,
C.getTargetInfo().getMaxPointerWidth());
Int8PtrTy = llvm::PointerType::get(LLVMContext,
C.getTargetAddressSpace(LangAS::Default));
+ UnqualPtrTy = llvm::PointerType::getUnqual(LLVMContext);
const llvm::DataLayout &DL = M.getDataLayout();
AllocaInt8PtrTy =
llvm::PointerType::get(LLVMContext, DL.getAllocaAddrSpace());
diff --git a/clang/lib/CodeGen/CodeGenTypeCache.h b/clang/lib/CodeGen/CodeGenTypeCache.h
index e273ebe3b060f..6fe7c669be9ab 100644
--- a/clang/lib/CodeGen/CodeGenTypeCache.h
+++ b/clang/lib/CodeGen/CodeGenTypeCache.h
@@ -51,15 +51,17 @@ struct CodeGenTypeCache {
llvm::IntegerType *PtrDiffTy;
};
- /// void*, void** in the target's default address space (often 0)
+ /// void*, void** in the target's default address space (often 0, but not
+ /// always)
union {
- llvm::PointerType *UnqualPtrTy;
llvm::PointerType *VoidPtrTy;
llvm::PointerType *Int8PtrTy;
llvm::PointerType *VoidPtrPtrTy;
llvm::PointerType *Int8PtrPtrTy;
};
+ llvm::PointerType *UnqualPtrTy;
+
/// void* in alloca address space
union {
llvm::PointerType *AllocaVoidPtrTy;
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 7dc2eaf1e9f75..68c52e5ab244b 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -5039,7 +5039,7 @@ 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.Int8PtrTy;
// Check for a copy expression. If we don't have a copy expression,
// that means a trivial copy is okay.
|
Thanks for the question! I do agree clang should mostly not be using the hardcoded address-space 0 (besides some exceptions*).On the LLVM side by convention *One example where address-space 0 is expected is for the elmenets of the |
|
So on LLVM side it is (often) assumed that the "default address space" always is AS=0. But on clang side targets can map DefaultAS to a non zero address space. I wonder if that inconsistency is a bigger problem than addressed by this patch. We for example have this in DerivedTypes.h: The direction of this patch seem to be that an unqualified pointer should be defined as having address space zero, rather than pointing to the default address space. Although I guess by unqualified we mean that it has no address space (so we only use zero as we need to use some value). Maybe that is OK, but then I think we for example should updated the comments in DerivedTypes.h to make that clear. |
I agree. The problem is certainly bigger and it happens to work since, most of the time, the default-AS is 0.
I should rewrite that comment. What do you think about |
|
I think on the rare occasion where we actually want addrspace 0, regardless of the target's addrspace preferences, I'd prefer to change it to explicitly request addrspace 0. |
I guess the question/ambiguity is about what "unqual" means. I'd imagine that we want to say something like But that may be a bit controversial given how many PointerType::getUnqual calls there is a in the code base that seem to assume the current wording (that it returns a pointer to "default address space"). And then it is even more confusing since in reality it seem to be that getUnqual always return an opaque pointer in address space zero, which for certain targets isn't the default address space. If we want "unqual" to refer to some kind of generic unnumbered address space, then we probably want to update lots of getUnqual calls to specify address space explicitly (possibly with some new wrappers for selecting address space zero or the targets default address space). |
|
Just to propose another approach to the problem, it really looks like the issue is mainly a naming issue more than anything. We could rename |
|
Sure, that seems like a good approach. |
Yes, this is a hack from the beginning of OpenCL. Instead of changing the default address space, it would be better if clang treated OpenCL 1.x as implicitly adding the private qualifier on pointers |
jmmartinez
force-pushed
the
fix/unqual-is-unqual
branch
from
449e8c8 to
96a74c6
Compare
jmmartinez
changed the title
|
Changed to rename only, and update title and description |
jmmartinez
force-pushed
the
fix/unqual-is-unqual
branch
2 times, most recently
from
5c9a103 to
6897a57
Compare
UnqualPtrTy didn't always match llvm::PointerType::getUnqual: sometimes it returned a pointer that is not in addrspace 0 (notably for SPIRV). Since UnqualPtrTy was used as the "generic" or "default" pointer type, this patch renames it to DefaultPtrTy to avoid confusion with LLVM's PointerType::getUnqual.
jmmartinez
force-pushed
the
fix/unqual-is-unqual
branch
from
6897a57 to
3a517f0
Compare
|
@bjope does this last version of the PR look good for you? |
|
Thanks for your help ! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/17768 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/26255 Here is the relevant piece of the build log for the reference |
6 participants
UnqualPtrTydidn't always matchllvm::PointerType::getUnqual: sometimes it returned a pointer that is not in address space 0 (notably for SPIRV).Since
UnqualPtrTywas used as the "generic" or "default" pointer type, this patch renames it toDefaultPtrTyto avoid confusion with LLVM'sPointerType::getUnqual.