[SYCL] Enable template parameter support for ii and max_concurrency attributes #811
Conversation
…ttributes Signed-off-by: Viktoria Maksimova <[email protected]>
Signed-off-by: Viktoria Maksimova <[email protected]>
Signed-off-by: Viktoria Maksimova <[email protected]>
bader
changed the title
Signed-off-by: Viktoria Maksimova <[email protected]>
| @@ -229,6 +229,36 @@ void ivdep_dependent() { | |||
| }; | |||
| } | |||
|
|
|||
| template <int A, int B, int C> | |||
There was a problem hiding this comment.
Can you test a non-dependent attribute in a dependent context? I have a feeling that the transforms running on a non-dependent version will cause the int value to be lost.
There was a problem hiding this comment.
You're right, int value is lost in this case. Do you have any suggestions why it happens or how to handle this? As I see, it requires more than one-line fix, but unfortunately I have no idea where the error can be hidden.
There was a problem hiding this comment.
I think the best way about this is to just remove the IntervalValue field from the attribute. I don't think it is necessary anyway. It'll simplify a bunch of code, and result in only the Expr needing to be handled everywhere. It means you need to do a touch more work in CGLoopInfo (call isIntegerConstantExpr() on the expr to get the integer value out), but it simplifies the rest of the patch a bunch.
Signed-off-by: Viktoria Maksimova <[email protected]>
clang/lib/CodeGen/CGLoopInfo.cpp
Outdated
| unsigned ValueInt = 0; | ||
| if (auto *E = IntelFPGAII->getIntervalExpr()) { | ||
| llvm::APSInt ArgVal(32); | ||
| if (E->isIntegerConstantExpr(ArgVal, Ctx)) { |
There was a problem hiding this comment.
This shouldn't be an 'if', it should absolutely be true (since you made sure of it before). This should assert. Typically we do something like:
bool Valid = E->IsIntegerConstantExpr(ArgVal, Ctx);
assert(Valid && "Not a ConstantExpr?");(void)Valid;
clang/lib/CodeGen/CGLoopInfo.cpp
Outdated
| if (ValueInt > 0) | ||
| setSYCLIInterval(ValueInt); | ||
| unsigned ValueInt = 0; | ||
| if (auto *E = IntelFPGAII->getIntervalExpr()) { |
There was a problem hiding this comment.
What codegen are we doing if there ISN'T an expression?
Also, const auto should be possible.
There was a problem hiding this comment.
Is it no codegen if there is no expression?
There was a problem hiding this comment.
I guess not? But this should likely be checked in SEMA and asserted here. Also, can you write a SEMA test for 'no parameters passed'? That seems like it should be an error case as well.
There was a problem hiding this comment.
Agree with that, no need to double-check.
About test - I see that the test has already have cases for no parameters in boo() function
There was a problem hiding this comment.
I see, thanks for pointing that out.
clang/lib/CodeGen/CGLoopInfo.cpp
Outdated
| } | ||
|
|
||
| if (IntelFPGAMaxConcurrency) { | ||
| setSYCLMaxConcurrencyEnable(); | ||
| setSYCLMaxConcurrencyNThreads(IntelFPGAMaxConcurrency->getNThreads()); | ||
| unsigned ValueInt = 0; |
There was a problem hiding this comment.
Same comments as above.
clang/lib/CodeGen/CGLoopInfo.cpp
Outdated
| llvm::APSInt ArgVal(32); | ||
| if (E->isIntegerConstantExpr(ArgVal, Ctx)) { | ||
| ValueInt = ArgVal.getSExtValue(); | ||
| if (ValueInt > 0) |
There was a problem hiding this comment.
You check this condition in SEMA right? No need to do it again.
| const SYCLIntelFPGAIIAttr *TemplateInstantiator::TransformSYCLIntelFPGAIIAttr( | ||
| const SYCLIntelFPGAIIAttr *II) { | ||
| Expr *TransformedExpr = | ||
| II->getIntervalExpr() |
There was a problem hiding this comment.
The ternary isn't necessary, TransformExpr returns nullptr if it receives it.
There was a problem hiding this comment.
Ternary is for expression checking. getIntervalExpr() can be nullptr, so .get() will cause segfault in this way.
There was a problem hiding this comment.
I don't believe that is correct. getDerived.TransformExpr(nullptr) would return an ExprResult containing a nullptr. The 'get' function should just return that same pointer: https://clang.llvm.org/doxygen/Ownership_8h_source.html#l00170
Thus, if you do getDerived().TransformExpr(II->getIntervalExpr()).get() you will get either the transformed expr, or nullptr.
There was a problem hiding this comment.
Oh, I see. Thank you for correction
Signed-off-by: Viktoria Maksimova <[email protected]>
No description provided.