[SYCL] Correction to constant-size array recognition. #2072
Conversation
Signed-off-by: rdeodhar <[email protected]>
rdeodhar
requested review from
elizabethandrews,
Fznamznon and
premanandrao
as code owners
There was a problem hiding this comment.
I'm not sure that I fully understand what is happening here. @erichkeane , could you please help with review?
Also, @rdeodhar, do you have a test for this change?
| @@ -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 = | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 > ( ))
There was a problem hiding this comment.
Ah, looks like there is a specific one for ArrayType: https://clang.llvm.org/doxygen/classclang_1_1ASTContext.html#a54d2386e12cd16689dd812d63b85560f
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think so? It does some canonicalization by calling getSplitDesugaredType, so that would imply to me that it removes typedef-type sugaring.
There was a problem hiding this comment.
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.
| return checkNotCopyableToKernel(FD, ET); | ||
| } | ||
| QualType FCT = FieldTy.getUnqualifiedType().getCanonicalType(); | ||
| if (isa<ConstantArrayType>(FCT)) |
There was a problem hiding this comment.
The above suggestion allows you to do what you were doing above here as well.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
See #2081 |
|
This one is covered by #2082 . Can we close it? |
|
Yes, this can be closed, and has been. |
Compilation unit can be translated earlier, e.g. in `transEntryPoint()`. We should save the translated LLVM value to be used further. Original commit: KhronosGroup/SPIRV-LLVM-Translator@803e528
Compilation unit can be translated earlier, e.g. in `transEntryPoint()`. We should save the translated LLVM value to be used further. Original commit: KhronosGroup/SPIRV-LLVM-Translator@803e528
Signed-off-by: rdeodhar [email protected]