[SYCL] Remove arbitrary range Part 1 #3133
Conversation
Remove arbitrary range for SYCLIntelSchedulerTargetFmaxMhz and SYCLIntelLoopFuse attributes. Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
| // CHECK: SYCLIntelSchedulerTargetFmaxMhzAttr {{.*}} | ||
| // CHECK-NEXT: ConstantExpr {{.*}} 'int' |
There was a problem hiding this comment.
I think ConstantExpr is no longer generated here because of a difference in how attribute expression is obtained in addIntelSYCLSingleArgFunctionAttr vs AddOneConstantValueAttr. The latter calls VerifyIntegerConstantExpression which generates this AST.
There was a problem hiding this comment.
@AaronBallman @premanandrao Should the ConstantExpr be generated. It isn't consistently generated for IntelSYCL attributes, which is why I removed it here.
There was a problem hiding this comment.
I think it should be -- see my reasoning in #3134 (comment)
There was a problem hiding this comment.
Ok. Thanks for the review! I'll make the change!
| if (CI.getParsedKind() == ParsedAttr::AT_SYCLIntelSchedulerTargetFmaxMhz) { | ||
| if (ArgInt < 0) { | ||
| Diag(E->getExprLoc(), diag::err_attribute_requires_positive_integer) | ||
| << CI.getAttrName() << /*non-negative*/ 1; | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
You can combine this inside addIntelSYCLSingleArgFunctionAttr () function :
if (CI.getParsedKind() == ParsedAttr::AT_SYCLIntelMaxGlobalWorkDim ||
CI.getParsedKind() == ParsedAttr::AT_SYCLIntelSchedulerTargetFmaxMhz ) {
if (ArgInt < 0) {
Diag(E->getExprLoc(), diag::err_attribute_requires_positive_integer)
<< CI.getAttrName() << /non-negative/ 1;
return;
}
There was a problem hiding this comment.
AT_SYCLIntelMaxGlobalWorkDim also has an upper-bound check, which is why I kept AT_SYCLIntelSchedulerTargetFmaxMhz in a separate check. There are actually also more FPGA attributes I am adding irefactoring in another patch, which will change this function even more. I will refactor this function then if that sounds ok to you.
There was a problem hiding this comment.
We definitely need to keep upper bound check for max_global_work_dim kernel attribute (which has a natural limitation of '3'). Several diagnostics and work_gorup_size attributes are dependent on this.
I agree with @MrSidims comments here #2911 (comment)
We should only remove the arbitrary upper bound here:
#2911 (comment)
There was a problem hiding this comment.
Yes we need to keep the upper bound of 3 for max_global_work_dim kernel attribute. I am removing only arbitrary upper bounds of 1024*1024 for the attributes mentioned here - #2911.
There was a problem hiding this comment.
addIntelSYCLSingleArgFunctionAttr () - I added this function to handle template parameter support for SYCL FPGA function attributes. So we can add other new function attributes in future here.
The following attributes below are FPGA memory attributes, i think it would be better if we add something like addIntelSingleArgMemoryAttr () instead of adding them inside addIntelSYCLSingleArgFunctionAttr ().
IntelFPGAPrivateCopies
IntelFPGABankBits
1 - 1024*1024
IntelFPGABankWidth
IntelFPGANumBanks
IntelFPGAMaxReplicates
@AaronBallman any thought?
There was a problem hiding this comment.
Thanks for taking a look @smanna12 . I do plan to refactor the functions when I work on the IntelFPGA attributes. You can see #3134. I've uploaded a draft PR (with just one attribute. Others will follow) to get initial feedback from @AaronBallman.
|
Replaced by - #3134 |
Remove arbitrary range for SYCLIntelSchedulerTargetFmaxMhz and
SYCLIntelLoopFuse attributes.
This patch partially addresses #2911
Signed-off-by: Elizabeth Andrews elizabeth.andrews@intel.com