[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
arichardson created this revision.
Herald added subscribers: Anastasia, nhaehnle, jholewinski.
Currently both clang AST address spaces and target specific address spaces
are represented as unsigned which can lead to subtle errors if the wrong
type is passed. It is especially confusing in the CodeGen files as it is
not possible to see what kind of address space should be passed to a
function without looking at the implementation.
I originally made this change for our LLVM fork for the CHERI architecture
where we make extensive use of address spaces to differentiate between
capabilities and pointers. When merging the upstream changes I usually
run into some test failures or runtime crashes because the wrong kind of
address space is passed to a function. By converting the LangAS enum to a
C++11 we can catch these errors at compile time. Additionally, it is now
obvious from the function signature which kind of address space it expects.
I found the following errors while writing this patch:
- ItaniumRecordLayoutBuilder::LayoutField was passing a clang AST address space
to TargetInfo::getPointer{Width,Align}()
- TypePrinter::printAttributedAfter() was printing the numeric value of the
clang AST address space instead of the target address space
- initializeForBlockHeader() in CGBlocks.cpp was passing LangAS::opencl_generic
to TargetInfo::getPointer{Width,Align}()
- CodeGenFunction::EmitBlockLiteral() was passing a AST address space to
TargetInfo::getPointerWidth()
- clang_getAddressSpace() was returning either a LangAS or a target address
address space. I made it consistently return a target address space
- CGOpenMPRuntimeNVPTX::translateParameter() passed a target address space to
Qualifiers::addAddressSpace()
- CGOpenMPRuntimeNVPTX::getParameterAddress() was using
llvm::Type::getPointerTo() with a AST address space
Other than this the patch should not cause any functional changes.
Repository:
rL LLVM
https://reviews.llvm.org/D38816
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
include/clang/Basic/AddressSpaces.h
include/clang/Basic/TargetInfo.h
lib/AST/ASTContext.cpp
lib/AST/ItaniumMangle.cpp
lib/AST/RecordLayoutBuilder.cpp
lib/AST/TypePrinter.cpp
lib/Basic/TargetInfo.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/Basic/Targets/AMDGPU.h
lib/CodeGen/CGBlocks.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExprConstant.cpp
lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
lib/CodeGen/CGValue.h
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/CodeGenModule.h
lib/CodeGen/CodeGenTypeCache.h
lib/CodeGen/ConstantEmitter.h
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclObjC.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaTemplateDeduction.cpp
lib/Sema/SemaType.cpp
tools/libclang/CXType.cpp
Index: tools/libclang/CXType.cpp
===
--- tools/libclang/CXType.cpp
+++ tools/libclang/CXType.cpp
@@ -398,12 +398,8 @@
unsigned clang_getAddressSpace(CXType CT) {
QualType T = GetQualType(CT);
-
- // For non language-specific address space, use separate helper function.
- if (T.getAddressSpace() >= LangAS::FirstTargetAddressSpace) {
-return T.getQualifiers().getAddressSpaceAttributePrintValue();
- }
- return T.getAddressSpace();
+ ASTContext &Ctx = cxtu::getASTUnit(GetTU(CT))->getASTContext();
+ return Ctx.getTargetAddressSpace(T);
}
CXString clang_getTypedefName(CXType CT) {
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -5632,7 +5632,7 @@
// If this type is already address space qualified, reject it.
// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
// by qualifiers for two or more different address spaces."
-if (T.getAddressSpace()) {
+if (T.getAddressSpace() != LangAS::Default) {
Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
return QualType();
}
@@ -5656,15 +5656,16 @@
}
llvm::APSInt max(addrSpace.getBitWidth());
-max = Qualifiers::MaxAddressSpace - LangAS::FirstTargetAddressSpace;
+max =
+Qualifiers::MaxAddressSpace - (unsigned)LangAS::FirstTargetAddressSpace;
if (addrSpace > max) {
Diag(AttrLoc, diag::err_attribute_address_space_too_high)
<< (unsigned)max.getZExtValue() << AddrSpace->getSourceRange();
return QualType();
}
-unsigned ASIdx = static_cast(addrSpace.getZExtValue()) +
- LangAS::FirstTargetAddressSpace;
+LangAS ASIdx = LanguageAS::fromTargetAS(
+static_cast(addrSpace.getZExtValue()));
return Context.getAddrSpaceQualType(T, ASIdx);
}
@@ -5690,7 +5691,7 @@
// If this type is already address space qualified, reject it.
// ISO/IEC TR 18
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
jlebar added a comment.
My only regret is that I have but one +1 to give to this patch.
Comment at: include/clang/Basic/AddressSpaces.h:51
+namespace LanguageAS {
/// The type of a lookup table which maps from language-specific address spaces
I wonder if you need this namespace? LangAS right next to LanguageAS reads
strangely to me -- "what's the difference?".
I guess you'd need to rename Map and fromTargetAS, but the other two members
are probably OK?
Repository:
rL LLVM
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
arichardson added inline comments.
Comment at: include/clang/Basic/AddressSpaces.h:51
+namespace LanguageAS {
/// The type of a lookup table which maps from language-specific address spaces
jlebar wrote:
> I wonder if you need this namespace? LangAS right next to LanguageAS reads
> strangely to me -- "what's the difference?".
>
> I guess you'd need to rename Map and fromTargetAS, but the other two members
> are probably OK?
The only reason I added this namespace is that I wasn't sure whether having
those functions in the clang namespace is acceptable.
Not quite sure what to call the functions though. `langASFromTargetAS`?
Repository:
rL LLVM
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
jlebar added a comment. > The only reason I added this namespace is that I wasn't sure whether having > those functions in the clang namespace is acceptable. Maybe someone else will object, or suggest an existing namespace they should be in. FWIW I think it's fine. > Not quite sure what to call the functions though. langASFromTargetAS? sgtm! Repository: rL LLVM https://reviews.llvm.org/D38816 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
arichardson updated this revision to Diff 118784.
arichardson added a comment.
Removed the additional namespace
https://reviews.llvm.org/D38816
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
include/clang/Basic/AddressSpaces.h
include/clang/Basic/TargetInfo.h
lib/AST/ASTContext.cpp
lib/AST/ItaniumMangle.cpp
lib/AST/RecordLayoutBuilder.cpp
lib/AST/TypePrinter.cpp
lib/Basic/TargetInfo.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/Basic/Targets/AMDGPU.h
lib/CodeGen/CGBlocks.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExprConstant.cpp
lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
lib/CodeGen/CGValue.h
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/CodeGenModule.h
lib/CodeGen/CodeGenTypeCache.h
lib/CodeGen/ConstantEmitter.h
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclObjC.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaTemplateDeduction.cpp
lib/Sema/SemaType.cpp
tools/libclang/CXType.cpp
Index: tools/libclang/CXType.cpp
===
--- tools/libclang/CXType.cpp
+++ tools/libclang/CXType.cpp
@@ -398,12 +398,8 @@
unsigned clang_getAddressSpace(CXType CT) {
QualType T = GetQualType(CT);
-
- // For non language-specific address space, use separate helper function.
- if (T.getAddressSpace() >= LangAS::FirstTargetAddressSpace) {
-return T.getQualifiers().getAddressSpaceAttributePrintValue();
- }
- return T.getAddressSpace();
+ ASTContext &Ctx = cxtu::getASTUnit(GetTU(CT))->getASTContext();
+ return Ctx.getTargetAddressSpace(T);
}
CXString clang_getTypedefName(CXType CT) {
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -5632,7 +5632,7 @@
// If this type is already address space qualified, reject it.
// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
// by qualifiers for two or more different address spaces."
-if (T.getAddressSpace()) {
+if (T.getAddressSpace() != LangAS::Default) {
Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
return QualType();
}
@@ -5656,15 +5656,16 @@
}
llvm::APSInt max(addrSpace.getBitWidth());
-max = Qualifiers::MaxAddressSpace - LangAS::FirstTargetAddressSpace;
+max =
+Qualifiers::MaxAddressSpace - (unsigned)LangAS::FirstTargetAddressSpace;
if (addrSpace > max) {
Diag(AttrLoc, diag::err_attribute_address_space_too_high)
<< (unsigned)max.getZExtValue() << AddrSpace->getSourceRange();
return QualType();
}
-unsigned ASIdx = static_cast(addrSpace.getZExtValue()) +
- LangAS::FirstTargetAddressSpace;
+LangAS ASIdx =
+LangASFromTargetAS(static_cast(addrSpace.getZExtValue()));
return Context.getAddrSpaceQualType(T, ASIdx);
}
@@ -5690,7 +5691,7 @@
// If this type is already address space qualified, reject it.
// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified by
// qualifiers for two or more different address spaces."
- if (Type.getAddressSpace()) {
+ if (Type.getAddressSpace() != LangAS::Default) {
S.Diag(Attr.getLoc(), diag::err_attribute_address_multiple_qualifiers);
Attr.setInvalid();
return;
@@ -5704,7 +5705,7 @@
return;
}
- unsigned ASIdx;
+ LangAS ASIdx;
if (Attr.getKind() == AttributeList::AT_AddressSpace) {
// Check the attribute arguments.
@@ -5754,7 +5755,8 @@
ASIdx = LangAS::opencl_generic; break;
default:
assert(Attr.getKind() == AttributeList::AT_OpenCLPrivateAddressSpace);
- ASIdx = 0; break;
+ ASIdx = LangAS::Default;
+ break;
}
Type = S.Context.getAddrSpaceQualType(Type, ASIdx);
@@ -7167,7 +7169,7 @@
// Pointers that are declared without pointing to a named address space point
// to the generic address space.
if (state.getSema().getLangOpts().OpenCLVersion >= 200 &&
- !hasOpenCLAddressSpace && type.getAddressSpace() == 0 &&
+ !hasOpenCLAddressSpace && type.getAddressSpace() == LangAS::Default &&
(TAL == TAL_DeclSpec || TAL == TAL_DeclChunk)) {
Declarator &D = state.getDeclarator();
if (state.getCurrentChunkIndex() > 0 &&
Index: lib/Sema/SemaTemplateDeduction.cpp
===
--- lib/Sema/SemaTemplateDeduction.cpp
+++ lib/Sema/SemaTemplateDeduction.cpp
@@ -1870,11 +1870,10 @@
Deduced);
}
- if (Arg.getAddressSpace() >= LangAS::FirstTargetAddressSpace) {
+ if (isTargetAddressSpace(Arg.getAddressSpace())) {
llvm::APSInt ArgAddressSpace(S.Context.getTypeSize(S.Context.IntTy),
false);
-ArgAddressSpace =
-(Arg.getAddressS
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
Anastasia added inline comments.
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
-OS << T->getEquivalentType().getAddressSpace();
+OS << T->getEquivalentType()
+ .getQualifiers()
Why do we need this change?
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
arichardson added inline comments.
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
-OS << T->getEquivalentType().getAddressSpace();
+OS << T->getEquivalentType()
+ .getQualifiers()
Anastasia wrote:
> Why do we need this change?
`__attribute__((address_space(n)))` is a target address space and not a
language address space like `LangAS::opencl_generic`. Isn't
`Qualifiers::getAddressSpaceAttributePrintValue()` meant exactly for this use
case?
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
bader accepted this revision. bader added a comment. This revision is now accepted and ready to land. @arichardson, great work! Thanks a lot! https://reviews.llvm.org/D38816 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
yaxunl added inline comments.
Comment at: include/clang/Basic/AddressSpaces.h:66
+inline LangAS LangASFromTargetAS(unsigned TargetAS) {
+ return static_cast((TargetAS) +
how about `getLangASFromTargetAS` ? It is preferred to start with small letters.
Comment at: tools/libclang/CXType.cpp:402
+ ASTContext &Ctx = cxtu::getASTUnit(GetTU(CT))->getASTContext();
+ return Ctx.getTargetAddressSpace(T);
}
Is this function suppose to return AST address space or target address space?
Some targets e.g. x86 maps all AST address spaces to 0. Returning target
address space will not let the client unable to differentiate different address
spaces in the source language.
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
arichardson planned changes to this revision.
arichardson added inline comments.
Comment at: include/clang/Basic/AddressSpaces.h:66
+inline LangAS LangASFromTargetAS(unsigned TargetAS) {
+ return static_cast((TargetAS) +
yaxunl wrote:
> how about `getLangASFromTargetAS` ? It is preferred to start with small
> letters.
Sounds good, I'll do that.
Comment at: tools/libclang/CXType.cpp:402
+ ASTContext &Ctx = cxtu::getASTUnit(GetTU(CT))->getASTContext();
+ return Ctx.getTargetAddressSpace(T);
}
yaxunl wrote:
> Is this function suppose to return AST address space or target address space?
>
> Some targets e.g. x86 maps all AST address spaces to 0. Returning target
> address space will not let the client unable to differentiate different
> address spaces in the source language.
I am not entirely sure what the correct return value is here because the
current implementation returns either the LanguageAS or `LangAS -
LangAS::FirstTargetAddressSpace` which can also overlap. So possibly it should
just always returning the AST address space?
I think for now I will just keep the current behaviour with a FIXME and create
a followup patch.
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
yaxunl added inline comments. Comment at: tools/libclang/CXType.cpp:402 + ASTContext &Ctx = cxtu::getASTUnit(GetTU(CT))->getASTContext(); + return Ctx.getTargetAddressSpace(T); } arichardson wrote: > yaxunl wrote: > > Is this function suppose to return AST address space or target address > > space? > > > > Some targets e.g. x86 maps all AST address spaces to 0. Returning target > > address space will not let the client unable to differentiate different > > address spaces in the source language. > I am not entirely sure what the correct return value is here because the > current implementation returns either the LanguageAS or `LangAS - > LangAS::FirstTargetAddressSpace` which can also overlap. So possibly it > should just always returning the AST address space? > > I think for now I will just keep the current behaviour with a FIXME and > create a followup patch. > That's fine. Thanks. https://reviews.llvm.org/D38816 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
arichardson updated this revision to Diff 118915.
arichardson edited the summary of this revision.
arichardson added a comment.
This revision is now accepted and ready to land.
- Keep old behaviour for clang_getAddressSpace()
- renamed to getLangASFromTargetAS()
- rebased on latest trunk
https://reviews.llvm.org/D38816
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
include/clang/Basic/AddressSpaces.h
include/clang/Basic/TargetInfo.h
lib/AST/ASTContext.cpp
lib/AST/ItaniumMangle.cpp
lib/AST/RecordLayoutBuilder.cpp
lib/AST/TypePrinter.cpp
lib/Basic/TargetInfo.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/Basic/Targets/AMDGPU.h
lib/CodeGen/CGBlocks.cpp
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExprConstant.cpp
lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
lib/CodeGen/CGValue.h
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/CodeGenModule.h
lib/CodeGen/CodeGenTypeCache.h
lib/CodeGen/ConstantEmitter.h
lib/CodeGen/TargetInfo.cpp
lib/CodeGen/TargetInfo.h
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclObjC.cpp
lib/Sema/SemaExpr.cpp
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaTemplateDeduction.cpp
lib/Sema/SemaType.cpp
tools/libclang/CXType.cpp
Index: tools/libclang/CXType.cpp
===
--- tools/libclang/CXType.cpp
+++ tools/libclang/CXType.cpp
@@ -403,7 +403,10 @@
if (T.getAddressSpace() >= LangAS::FirstTargetAddressSpace) {
return T.getQualifiers().getAddressSpaceAttributePrintValue();
}
- return T.getAddressSpace();
+ // FIXME: this function returns either a LangAS or a target AS
+ // Those values can overlap which makes this function rather unpredictable
+ // for any caller
+ return (unsigned)T.getAddressSpace();
}
CXString clang_getTypedefName(CXType CT) {
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -5631,7 +5631,7 @@
// If this type is already address space qualified, reject it.
// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
// by qualifiers for two or more different address spaces."
-if (T.getAddressSpace()) {
+if (T.getAddressSpace() != LangAS::Default) {
Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
return QualType();
}
@@ -5655,15 +5655,16 @@
}
llvm::APSInt max(addrSpace.getBitWidth());
-max = Qualifiers::MaxAddressSpace - LangAS::FirstTargetAddressSpace;
+max =
+Qualifiers::MaxAddressSpace - (unsigned)LangAS::FirstTargetAddressSpace;
if (addrSpace > max) {
Diag(AttrLoc, diag::err_attribute_address_space_too_high)
<< (unsigned)max.getZExtValue() << AddrSpace->getSourceRange();
return QualType();
}
-unsigned ASIdx = static_cast(addrSpace.getZExtValue()) +
- LangAS::FirstTargetAddressSpace;
+LangAS ASIdx =
+getLangASFromTargetAS(static_cast(addrSpace.getZExtValue()));
return Context.getAddrSpaceQualType(T, ASIdx);
}
@@ -5689,7 +5690,7 @@
// If this type is already address space qualified, reject it.
// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified by
// qualifiers for two or more different address spaces."
- if (Type.getAddressSpace()) {
+ if (Type.getAddressSpace() != LangAS::Default) {
S.Diag(Attr.getLoc(), diag::err_attribute_address_multiple_qualifiers);
Attr.setInvalid();
return;
@@ -5703,7 +5704,7 @@
return;
}
- unsigned ASIdx;
+ LangAS ASIdx;
if (Attr.getKind() == AttributeList::AT_AddressSpace) {
// Check the attribute arguments.
@@ -7036,7 +7037,7 @@
(T->isVoidType() && !IsPointee))
return;
- unsigned ImpAddr;
+ LangAS ImpAddr;
// Put OpenCL automatic variable in private address space.
// OpenCL v1.2 s6.5:
// The default address space name for arguments to a function in a
Index: lib/Sema/SemaTemplateDeduction.cpp
===
--- lib/Sema/SemaTemplateDeduction.cpp
+++ lib/Sema/SemaTemplateDeduction.cpp
@@ -1870,11 +1870,10 @@
Deduced);
}
- if (Arg.getAddressSpace() >= LangAS::FirstTargetAddressSpace) {
+ if (isTargetAddressSpace(Arg.getAddressSpace())) {
llvm::APSInt ArgAddressSpace(S.Context.getTypeSize(S.Context.IntTy),
false);
-ArgAddressSpace =
-(Arg.getAddressSpace() - LangAS::FirstTargetAddressSpace);
+ArgAddressSpace = toTargetAddressSpace(Arg.getAddressSpace());
// Perform deduction on the pointer types.
if (Sema::TemplateDeductionResult Result =
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -2113,7 +
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
Anastasia added inline comments.
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
-OS << T->getEquivalentType().getAddressSpace();
+OS << T->getEquivalentType()
+ .getQualifiers()
arichardson wrote:
> Anastasia wrote:
> > Why do we need this change?
> `__attribute__((address_space(n)))` is a target address space and not a
> language address space like `LangAS::opencl_generic`. Isn't
> `Qualifiers::getAddressSpaceAttributePrintValue()` meant exactly for this use
> case?
Yes, I think there are some adjustment we do in this method to get the original
source value to be printed corerctly. Does this mean we have no tests that
caught this issue?
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
arichardson added inline comments.
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
-OS << T->getEquivalentType().getAddressSpace();
+OS << T->getEquivalentType()
+ .getQualifiers()
Anastasia wrote:
> arichardson wrote:
> > Anastasia wrote:
> > > Why do we need this change?
> > `__attribute__((address_space(n)))` is a target address space and not a
> > language address space like `LangAS::opencl_generic`. Isn't
> > `Qualifiers::getAddressSpaceAttributePrintValue()` meant exactly for this
> > use case?
> Yes, I think there are some adjustment we do in this method to get the
> original source value to be printed corerctly. Does this mean we have no
> tests that caught this issue?
Seems like it, all tests pass both with and without this patch.
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
Anastasia added inline comments.
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
-OS << T->getEquivalentType().getAddressSpace();
+OS << T->getEquivalentType()
+ .getQualifiers()
arichardson wrote:
> Anastasia wrote:
> > arichardson wrote:
> > > Anastasia wrote:
> > > > Why do we need this change?
> > > `__attribute__((address_space(n)))` is a target address space and not a
> > > language address space like `LangAS::opencl_generic`. Isn't
> > > `Qualifiers::getAddressSpaceAttributePrintValue()` meant exactly for this
> > > use case?
> > Yes, I think there are some adjustment we do in this method to get the
> > original source value to be printed corerctly. Does this mean we have no
> > tests that caught this issue?
> Seems like it, all tests pass both with and without this patch.
Strange considering that we have this attribute printed in some error messages
of some Sema tests. If I compile this code without your patch:
```
typedef int __attribute__((address_space(1))) int_1;
typedef int __attribute__((address_space(2))) int_2;
void f0(int_1 &);
void f0(const int_1 &);
void test_f0() {
int i;
static int_2 i2;
f0(i);
f0(i2);
}
```
I get the address spaces printed correctly inside the type:
note: candidate function not viable: 1st argument ('int_2' (aka
'__attribute__((address_space(2))) int')) is in address space 2, but parameter
must be in address space 1
Perhaps @yaxunl could comment further on whether this change is needed.
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
arichardson added inline comments.
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
-OS << T->getEquivalentType().getAddressSpace();
+OS << T->getEquivalentType()
+ .getQualifiers()
Anastasia wrote:
> arichardson wrote:
> > Anastasia wrote:
> > > arichardson wrote:
> > > > Anastasia wrote:
> > > > > Why do we need this change?
> > > > `__attribute__((address_space(n)))` is a target address space and not a
> > > > language address space like `LangAS::opencl_generic`. Isn't
> > > > `Qualifiers::getAddressSpaceAttributePrintValue()` meant exactly for
> > > > this use case?
> > > Yes, I think there are some adjustment we do in this method to get the
> > > original source value to be printed corerctly. Does this mean we have no
> > > tests that caught this issue?
> > Seems like it, all tests pass both with and without this patch.
> Strange considering that we have this attribute printed in some error
> messages of some Sema tests. If I compile this code without your patch:
>
> ```
> typedef int __attribute__((address_space(1))) int_1;
> typedef int __attribute__((address_space(2))) int_2;
>
> void f0(int_1 &);
> void f0(const int_1 &);
>
> void test_f0() {
> int i;
> static int_2 i2;
> f0(i);
> f0(i2);
> }
> ```
>
> I get the address spaces printed correctly inside the type:
> note: candidate function not viable: 1st argument ('int_2' (aka
> '__attribute__((address_space(2))) int')) is in address space 2, but
> parameter must be in address space 1
>
> Perhaps @yaxunl could comment further on whether this change is needed.
My guess is that it doesn't go through that switch statement but rather through
`Qualifiers::print()`. I'll try adding a llvm_unreachable() to see if there are
any tests that go down this path.
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
arichardson added inline comments.
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
-OS << T->getEquivalentType().getAddressSpace();
+OS << T->getEquivalentType()
+ .getQualifiers()
arichardson wrote:
> Anastasia wrote:
> > arichardson wrote:
> > > Anastasia wrote:
> > > > arichardson wrote:
> > > > > Anastasia wrote:
> > > > > > Why do we need this change?
> > > > > `__attribute__((address_space(n)))` is a target address space and not
> > > > > a language address space like `LangAS::opencl_generic`. Isn't
> > > > > `Qualifiers::getAddressSpaceAttributePrintValue()` meant exactly for
> > > > > this use case?
> > > > Yes, I think there are some adjustment we do in this method to get the
> > > > original source value to be printed corerctly. Does this mean we have
> > > > no tests that caught this issue?
> > > Seems like it, all tests pass both with and without this patch.
> > Strange considering that we have this attribute printed in some error
> > messages of some Sema tests. If I compile this code without your patch:
> >
> > ```
> > typedef int __attribute__((address_space(1))) int_1;
> > typedef int __attribute__((address_space(2))) int_2;
> >
> > void f0(int_1 &);
> > void f0(const int_1 &);
> >
> > void test_f0() {
> > int i;
> > static int_2 i2;
> > f0(i);
> > f0(i2);
> > }
> > ```
> >
> > I get the address spaces printed correctly inside the type:
> > note: candidate function not viable: 1st argument ('int_2' (aka
> > '__attribute__((address_space(2))) int')) is in address space 2, but
> > parameter must be in address space 1
> >
> > Perhaps @yaxunl could comment further on whether this change is needed.
> My guess is that it doesn't go through that switch statement but rather
> through `Qualifiers::print()`. I'll try adding a llvm_unreachable() to see if
> there are any tests that go down this path.
I just ran the clang tests with an llvm_unreachable() here and none of them
failed. So it seems like we don't have anything testing this code path.
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
yaxunl added inline comments.
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
-OS << T->getEquivalentType().getAddressSpace();
+OS << T->getEquivalentType()
+ .getQualifiers()
arichardson wrote:
> arichardson wrote:
> > Anastasia wrote:
> > > arichardson wrote:
> > > > Anastasia wrote:
> > > > > arichardson wrote:
> > > > > > Anastasia wrote:
> > > > > > > Why do we need this change?
> > > > > > `__attribute__((address_space(n)))` is a target address space and
> > > > > > not a language address space like `LangAS::opencl_generic`. Isn't
> > > > > > `Qualifiers::getAddressSpaceAttributePrintValue()` meant exactly
> > > > > > for this use case?
> > > > > Yes, I think there are some adjustment we do in this method to get
> > > > > the original source value to be printed corerctly. Does this mean we
> > > > > have no tests that caught this issue?
> > > > Seems like it, all tests pass both with and without this patch.
> > > Strange considering that we have this attribute printed in some error
> > > messages of some Sema tests. If I compile this code without your patch:
> > >
> > > ```
> > > typedef int __attribute__((address_space(1))) int_1;
> > > typedef int __attribute__((address_space(2))) int_2;
> > >
> > > void f0(int_1 &);
> > > void f0(const int_1 &);
> > >
> > > void test_f0() {
> > > int i;
> > > static int_2 i2;
> > > f0(i);
> > > f0(i2);
> > > }
> > > ```
> > >
> > > I get the address spaces printed correctly inside the type:
> > > note: candidate function not viable: 1st argument ('int_2' (aka
> > > '__attribute__((address_space(2))) int')) is in address space 2, but
> > > parameter must be in address space 1
> > >
> > > Perhaps @yaxunl could comment further on whether this change is needed.
> > My guess is that it doesn't go through that switch statement but rather
> > through `Qualifiers::print()`. I'll try adding a llvm_unreachable() to see
> > if there are any tests that go down this path.
> I just ran the clang tests with an llvm_unreachable() here and none of them
> failed. So it seems like we don't have anything testing this code path.
Sorry for the delay. This part of code is for printing the addr space of
AttributedType. Since it seems not used by any language yet, there is no test
for it. It is possible a non-target-specific address space being printed here
if a language chooses to use AttributedType to represent address space.
Therefore a proper fix would be isolate the code for printing address space
from Qualifiers::print and re-use it here so that addr space is printed in
consistent way no matter it is represented as qualifier or as AttributedType.
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
arichardson added inline comments.
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
-OS << T->getEquivalentType().getAddressSpace();
+OS << T->getEquivalentType()
+ .getQualifiers()
yaxunl wrote:
> arichardson wrote:
> > arichardson wrote:
> > > Anastasia wrote:
> > > > arichardson wrote:
> > > > > Anastasia wrote:
> > > > > > arichardson wrote:
> > > > > > > Anastasia wrote:
> > > > > > > > Why do we need this change?
> > > > > > > `__attribute__((address_space(n)))` is a target address space and
> > > > > > > not a language address space like `LangAS::opencl_generic`. Isn't
> > > > > > > `Qualifiers::getAddressSpaceAttributePrintValue()` meant exactly
> > > > > > > for this use case?
> > > > > > Yes, I think there are some adjustment we do in this method to get
> > > > > > the original source value to be printed corerctly. Does this mean
> > > > > > we have no tests that caught this issue?
> > > > > Seems like it, all tests pass both with and without this patch.
> > > > Strange considering that we have this attribute printed in some error
> > > > messages of some Sema tests. If I compile this code without your patch:
> > > >
> > > > ```
> > > > typedef int __attribute__((address_space(1))) int_1;
> > > > typedef int __attribute__((address_space(2))) int_2;
> > > >
> > > > void f0(int_1 &);
> > > > void f0(const int_1 &);
> > > >
> > > > void test_f0() {
> > > > int i;
> > > > static int_2 i2;
> > > > f0(i);
> > > > f0(i2);
> > > > }
> > > > ```
> > > >
> > > > I get the address spaces printed correctly inside the type:
> > > > note: candidate function not viable: 1st argument ('int_2' (aka
> > > > '__attribute__((address_space(2))) int')) is in address space 2, but
> > > > parameter must be in address space 1
> > > >
> > > > Perhaps @yaxunl could comment further on whether this change is needed.
> > > My guess is that it doesn't go through that switch statement but rather
> > > through `Qualifiers::print()`. I'll try adding a llvm_unreachable() to
> > > see if there are any tests that go down this path.
> > I just ran the clang tests with an llvm_unreachable() here and none of them
> > failed. So it seems like we don't have anything testing this code path.
> Sorry for the delay. This part of code is for printing the addr space of
> AttributedType. Since it seems not used by any language yet, there is no test
> for it. It is possible a non-target-specific address space being printed here
> if a language chooses to use AttributedType to represent address space.
> Therefore a proper fix would be isolate the code for printing address space
> from Qualifiers::print and re-use it here so that addr space is printed in
> consistent way no matter it is represented as qualifier or as AttributedType.
Thanks, that makes sense. To avoid breaking anything here I think it should be
part of a separate patch though.
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
yaxunl added inline comments.
Comment at: lib/AST/TypePrinter.cpp:1323
OS << "address_space(";
-OS << T->getEquivalentType().getAddressSpace();
+OS << T->getEquivalentType()
+ .getQualifiers()
arichardson wrote:
> yaxunl wrote:
> > arichardson wrote:
> > > arichardson wrote:
> > > > Anastasia wrote:
> > > > > arichardson wrote:
> > > > > > Anastasia wrote:
> > > > > > > arichardson wrote:
> > > > > > > > Anastasia wrote:
> > > > > > > > > Why do we need this change?
> > > > > > > > `__attribute__((address_space(n)))` is a target address space
> > > > > > > > and not a language address space like `LangAS::opencl_generic`.
> > > > > > > > Isn't `Qualifiers::getAddressSpaceAttributePrintValue()` meant
> > > > > > > > exactly for this use case?
> > > > > > > Yes, I think there are some adjustment we do in this method to
> > > > > > > get the original source value to be printed corerctly. Does this
> > > > > > > mean we have no tests that caught this issue?
> > > > > > Seems like it, all tests pass both with and without this patch.
> > > > > Strange considering that we have this attribute printed in some error
> > > > > messages of some Sema tests. If I compile this code without your
> > > > > patch:
> > > > >
> > > > > ```
> > > > > typedef int __attribute__((address_space(1))) int_1;
> > > > > typedef int __attribute__((address_space(2))) int_2;
> > > > >
> > > > > void f0(int_1 &);
> > > > > void f0(const int_1 &);
> > > > >
> > > > > void test_f0() {
> > > > > int i;
> > > > > static int_2 i2;
> > > > > f0(i);
> > > > > f0(i2);
> > > > > }
> > > > > ```
> > > > >
> > > > > I get the address spaces printed correctly inside the type:
> > > > > note: candidate function not viable: 1st argument ('int_2' (aka
> > > > > '__attribute__((address_space(2))) int')) is in address space 2, but
> > > > > parameter must be in address space 1
> > > > >
> > > > > Perhaps @yaxunl could comment further on whether this change is
> > > > > needed.
> > > > My guess is that it doesn't go through that switch statement but rather
> > > > through `Qualifiers::print()`. I'll try adding a llvm_unreachable() to
> > > > see if there are any tests that go down this path.
> > > I just ran the clang tests with an llvm_unreachable() here and none of
> > > them failed. So it seems like we don't have anything testing this code
> > > path.
> > Sorry for the delay. This part of code is for printing the addr space of
> > AttributedType. Since it seems not used by any language yet, there is no
> > test for it. It is possible a non-target-specific address space being
> > printed here if a language chooses to use AttributedType to represent
> > address space. Therefore a proper fix would be isolate the code for
> > printing address space from Qualifiers::print and re-use it here so that
> > addr space is printed in consistent way no matter it is represented as
> > qualifier or as AttributedType.
> Thanks, that makes sense. To avoid breaking anything here I think it should
> be part of a separate patch though.
Sure. In this one probably keep the original behavior.
https://reviews.llvm.org/D38816
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
yaxunl accepted this revision. yaxunl added a comment. LGTM. Thanks! Great work! https://reviews.llvm.org/D38816 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38816: Convert clang::LangAS to a strongly typed enum
This revision was automatically updated to reflect the committed changes.
Closed by commit rL315871: Convert clang::LangAS to a strongly typed enum
(authored by arichardson).
Changed prior to commit:
https://reviews.llvm.org/D38816?vs=118915&id=119090#toc
Repository:
rL LLVM
https://reviews.llvm.org/D38816
Files:
cfe/trunk/include/clang/AST/ASTContext.h
cfe/trunk/include/clang/AST/Type.h
cfe/trunk/include/clang/Basic/AddressSpaces.h
cfe/trunk/include/clang/Basic/TargetInfo.h
cfe/trunk/lib/AST/ASTContext.cpp
cfe/trunk/lib/AST/ItaniumMangle.cpp
cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
cfe/trunk/lib/AST/TypePrinter.cpp
cfe/trunk/lib/Basic/TargetInfo.cpp
cfe/trunk/lib/Basic/Targets/AMDGPU.cpp
cfe/trunk/lib/Basic/Targets/AMDGPU.h
cfe/trunk/lib/CodeGen/CGBlocks.cpp
cfe/trunk/lib/CodeGen/CGDecl.cpp
cfe/trunk/lib/CodeGen/CGExprConstant.cpp
cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
cfe/trunk/lib/CodeGen/CGValue.h
cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.h
cfe/trunk/lib/CodeGen/CodeGenTypeCache.h
cfe/trunk/lib/CodeGen/ConstantEmitter.h
cfe/trunk/lib/CodeGen/TargetInfo.cpp
cfe/trunk/lib/CodeGen/TargetInfo.h
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaDeclAttr.cpp
cfe/trunk/lib/Sema/SemaDeclObjC.cpp
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/lib/Sema/SemaExprCXX.cpp
cfe/trunk/lib/Sema/SemaTemplateDeduction.cpp
cfe/trunk/lib/Sema/SemaType.cpp
cfe/trunk/tools/libclang/CXType.cpp
Index: cfe/trunk/tools/libclang/CXType.cpp
===
--- cfe/trunk/tools/libclang/CXType.cpp
+++ cfe/trunk/tools/libclang/CXType.cpp
@@ -403,7 +403,10 @@
if (T.getAddressSpace() >= LangAS::FirstTargetAddressSpace) {
return T.getQualifiers().getAddressSpaceAttributePrintValue();
}
- return T.getAddressSpace();
+ // FIXME: this function returns either a LangAS or a target AS
+ // Those values can overlap which makes this function rather unpredictable
+ // for any caller
+ return (unsigned)T.getAddressSpace();
}
CXString clang_getTypedefName(CXType CT) {
Index: cfe/trunk/include/clang/AST/Type.h
===
--- cfe/trunk/include/clang/AST/Type.h
+++ cfe/trunk/include/clang/AST/Type.h
@@ -328,32 +328,34 @@
}
bool hasAddressSpace() const { return Mask & AddressSpaceMask; }
- unsigned getAddressSpace() const { return Mask >> AddressSpaceShift; }
+ LangAS getAddressSpace() const {
+return static_cast(Mask >> AddressSpaceShift);
+ }
bool hasTargetSpecificAddressSpace() const {
-return getAddressSpace() >= LangAS::FirstTargetAddressSpace;
+return isTargetAddressSpace(getAddressSpace());
}
/// Get the address space attribute value to be printed by diagnostics.
unsigned getAddressSpaceAttributePrintValue() const {
auto Addr = getAddressSpace();
// This function is not supposed to be used with language specific
// address spaces. If that happens, the diagnostic message should consider
// printing the QualType instead of the address space value.
-assert(Addr == 0 || hasTargetSpecificAddressSpace());
-if (Addr)
- return Addr - LangAS::FirstTargetAddressSpace;
+assert(Addr == LangAS::Default || hasTargetSpecificAddressSpace());
+if (Addr != LangAS::Default)
+ return toTargetAddressSpace(Addr);
// TODO: The diagnostic messages where Addr may be 0 should be fixed
// since it cannot differentiate the situation where 0 denotes the default
// address space or user specified __attribute__((address_space(0))).
return 0;
}
- void setAddressSpace(unsigned space) {
-assert(space <= MaxAddressSpace);
+ void setAddressSpace(LangAS space) {
+assert((unsigned)space <= MaxAddressSpace);
Mask = (Mask & ~AddressSpaceMask)
| (((uint32_t) space) << AddressSpaceShift);
}
- void removeAddressSpace() { setAddressSpace(0); }
- void addAddressSpace(unsigned space) {
-assert(space);
+ void removeAddressSpace() { setAddressSpace(LangAS::Default); }
+ void addAddressSpace(LangAS space) {
+assert(space != LangAS::Default);
setAddressSpace(space);
}
@@ -1005,7 +1007,7 @@
}
/// Return the address space of this type.
- inline unsigned getAddressSpace() const;
+ inline LangAS getAddressSpace() const;
/// Returns gc attribute of this type.
inline Qualifiers::GC getObjCGCAttr() const;
@@ -1230,7 +1232,7 @@
}
bool hasAddressSpace() const { return Quals.hasAddressSpace(); }
- unsigned getAddressSpace() const { return Quals.getAddressSpace(); }
+ LangAS getAddressSpace() const { return Quals.getAddressSpace(); }
const Type *getBaseType() const { return BaseType; }
@@ -5654,7 +5656,7 @@
}
/// Return the address space of this type.
-inline unsigned QualType::getAddressSpace() co
