Skip to content

[SYCL] Correction to constant-size array recognition. #2072

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

Closed
wants to merge 1 commit into from
Closed

[SYCL] Correction to constant-size array recognition. #2072

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 14 additions & 13 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -797,7 +797,8 @@ static void VisitField(CXXRecordDecl *Owner, RangeTy &&Item, QualType ItemTy,
template <typename RangeTy, typename... Handlers>
static void VisitArrayElements(RangeTy Item, QualType FieldTy,
Handlers &... handlers) {
const ConstantArrayType *CAT = cast<ConstantArrayType>(FieldTy);
const ConstantArrayType *CAT =
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of casts throughout this, you want to be doing getas. https://clang.llvm.org/doxygen/classclang_1_1Type.html#adae68e1f4c85ede2d36da45fbefc48a2

This will make sure you get the unqualified desugared type.

Copy link
Contributor

@elizabethandrews elizabethandrews Jul 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if getAs works with ArrayType. While working on another patch I hit this assert -https://clang.llvm.org/doxygen/Type_8h_source.html#l07153

This was the code that failed:
if (const ConstantArrayType *CAT = FieldTy->getAs < ConstantArrayType > ( ))

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, looks like there is a specific one for ArrayType: https://clang.llvm.org/doxygen/classclang_1_1ASTContext.html#a54d2386e12cd16689dd812d63b85560f

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I was just looking into that. Do you know if that will work when checking elaborated types? It wasn't clear to me by looking at definition.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so? It does some canonicalization by calling getSplitDesugaredType, so that would imply to me that it removes typedef-type sugaring.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind. I was looking at the wrong definition. I think it will work. This is what the comment says -

 If the type is an instance of the specified class,
 /// return the Type pointer for the underlying maximally pretty type.  This
 /// is a member of ASTContext because this may need to do some amount of
 /// canonicalization, e.g. to move type qualifiers into the element type.

cast<ConstantArrayType>(FieldTy.getUnqualifiedType().getCanonicalType());
QualType ET = CAT->getElementType();
int64_t ElemCount = CAT->getSize().getSExtValue();
std::initializer_list<int>{(handlers.enterArray(), 0)...};
Expand Down Expand Up @@ -1005,10 +1006,10 @@ class SyclKernelFieldChecker
// Return true if not copyable, false if copyable.
bool checkNotCopyableToKernel(const FieldDecl *FD, const QualType &FieldTy) {
if (FieldTy->isArrayType()) {
if (const auto *CAT = dyn_cast<ConstantArrayType>(FieldTy)) {
QualType ET = CAT->getElementType();
return checkNotCopyableToKernel(FD, ET);
}
QualType FCT = FieldTy.getUnqualifiedType().getCanonicalType();
if (isa<ConstantArrayType>(FCT))
Copy link
Contributor

Choose a reason for hiding this comment

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

The above suggestion allows you to do what you were doing above here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getAsConstantArrayType is an ASTContext API. To use it, the Visitor functions will need to get the context from the handlers. The context is not currently available outside the handler classes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately it is really the only right way to do this... The way above loses context in a few ways that it shouldn't.

I think the visitor functions themselves should probably have a reference to Sema, and the handlers can capture what they need.

IMO, the visitors likely should all belong to a class. I'll submit a patch on which you can rebase in order to get this info.

return checkNotCopyableToKernel(
FD, cast<ConstantArrayType>(FCT)->getElementType());
return Diag.Report(FD->getLocation(),
diag::err_sycl_non_constant_array_type)
<< FieldTy;
Expand Down Expand Up @@ -1104,10 +1105,10 @@ class SyclKernelDeclCreator
size_t LastParamIndex = 0;

void addParam(const FieldDecl *FD, QualType FieldTy) {
const ConstantArrayType *CAT =
SemaRef.getASTContext().getAsConstantArrayType(FieldTy);
if (CAT)
FieldTy = CAT->getElementType();
if (FieldTy->isArrayType()) {
QualType FCT = FieldTy.getUnqualifiedType().getCanonicalType();
FieldTy = cast<ConstantArrayType>(FCT)->getElementType();
}
ParamDesc newParamDesc = makeParamDesc(FD, FieldTy);
addParam(newParamDesc, FieldTy);
}
Expand Down Expand Up @@ -1700,10 +1701,10 @@ class SyclKernelIntHeaderCreator
void addParam(const FieldDecl *FD, QualType ArgTy,
SYCLIntegrationHeader::kernel_param_kind_t Kind) {
uint64_t Size;
const ConstantArrayType *CAT =
SemaRef.getASTContext().getAsConstantArrayType(ArgTy);
if (CAT)
ArgTy = CAT->getElementType();
if (ArgTy->isArrayType()) {
QualType ACT = ArgTy.getUnqualifiedType().getCanonicalType();
ArgTy = cast<ConstantArrayType>(ACT)->getElementType();
}
Size = SemaRef.getASTContext().getTypeSizeInChars(ArgTy).getQuantity();
Header.addParamDesc(Kind, static_cast<unsigned>(Size),
static_cast<unsigned>(CurOffset));
Expand Down