[SYCL] Add validation + exception handling to SYCL_PARALLEL_FOR_RANGE_ROUNDING_PARAMS #19381
Conversation
|
@intel/llvm-reviewers-runtime PR ready for review, thanks in advance! |
| size_t &MinRange) { | ||
| static const char *RoundParams = BaseT::getRawValue(); | ||
| size_t &MinRange, bool ForceUpdate = false) { | ||
| const char *RoundParams = BaseT::getRawValue(); |
There was a problem hiding this comment.
Do we want to remove static from the declaration of *RoundParams ?
There was a problem hiding this comment.
I initially removed it because I figured the variable should be re-read every time, but in hindsight this whole statement should be moved elsewhere to avoid this being called everytime the function is ran...
There was a problem hiding this comment.
In hindsight's hindsight, if the env variable is empty getRawValue returns null; in which it actually makes sense the check is where it is
| if (Pos != std::string::npos) { | ||
| MF = std::stoi(Params.substr(0, Pos)); | ||
| if (Pos != std::string::npos && GuardedStoi(MF, Params.substr(0, Pos)) && | ||
| MF > 0) { |
There was a problem hiding this comment.
should this be MF >= 0 ( GTE instead of GT) ? 0 is a valid return from GuardedStoi, no? Or maybe 0 should not be valid in GuardedStoi.
There was a problem hiding this comment.
The way we currently have range rounding, MF=0 results in a division by zero error: This PR helps prevent the crash that would happen as a result.
I don't think it makes sense anyway, as according to https://github.com/intel/llvm/blob/970e2ef386e6287202cbaf69ff3871889bed5959/sycl/doc/EnvironmentVariables.md, the minimum factor is the "range that the rounded range should be a multiple of (Default 16)": Rounding parallel_for ranges to a multiple of 0 should probably not be possible. Thus, I have a test case to explicitly check that MF=0 should not be accepted.
No description provided.