[Clang][CodeGen] Make UnqualPtrTy match llvm::PointerType::getUnqual #163207
Conversation
The Int8PtrTy doesn't match the addresspace(0) for every platform (hello SPIRV!). Then, we end up with unexpected behaviour when we use UnqualPtrTy expecting an addresspace(0) ptr that matches llvm::PointerType::getUnqual, but we get something else.
Before, a ptr to function (addrspace(0)) was used. Now we use a pointer to the default address space, the generic addrspace(4). This used to work since before the unqual addrspace used to match the default address space.
llvmbot
added
clang
|
@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 |
5 participants
The
UnqualPtrTydoesn't match thellvm::PointerType::getUnqualfor every target.This patch makes the
UnqualPtrTyalways matchllvm::PointerType::getUnqual.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).