-
Notifications
You must be signed in to change notification settings - Fork 797
[SYCL] Add validation + exception handling to SYCL_PARALLEL_FOR_RANGE_ROUNDING_PARAMS #19381
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.
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
base: sycl
Are you sure you want to change the base?
Changes from all commits
cf5c4a7
a807d2e
eb2efc2
a757ff7
c056eb0
216f40d
b282348
766d0db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -199,35 +199,64 @@ template <> class SYCLConfig<SYCL_PARALLEL_FOR_RANGE_ROUNDING_PARAMS> { | |
private: | ||
public: | ||
static void GetSettings(size_t &MinFactor, size_t &GoodFactor, | ||
size_t &MinRange) { | ||
static const char *RoundParams = BaseT::getRawValue(); | ||
size_t &MinRange, bool ForceUpdate = false) { | ||
const char *RoundParams = BaseT::getRawValue(); | ||
if (RoundParams == nullptr) | ||
return; | ||
|
||
static bool ProcessedFactors = false; | ||
static bool FactorsAreValid = false; | ||
static size_t MF; | ||
static size_t GF; | ||
static size_t MR; | ||
if (!ProcessedFactors) { | ||
if (!ProcessedFactors || ForceUpdate) { | ||
auto GuardedStoi = [](size_t &val, const std::string &str) { | ||
try { | ||
int ParsedResult = std::stoi(str); | ||
if (ParsedResult < 0) | ||
return false; | ||
val = ParsedResult; | ||
return true; | ||
// Ignore parsing exceptions, but throw on unexpected exceptions: | ||
} catch (const std::invalid_argument &) { | ||
} catch (const std::out_of_range &) { | ||
} | ||
return false; | ||
}; | ||
|
||
// Parse optional parameters of this form (all values required): | ||
// MinRound:PreferredRound:MinRange | ||
std::string Params(RoundParams); | ||
size_t Pos = Params.find(':'); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
Params.erase(0, Pos + 1); | ||
Pos = Params.find(':'); | ||
if (Pos != std::string::npos) { | ||
GF = std::stoi(Params.substr(0, Pos)); | ||
if (Pos != std::string::npos && | ||
GuardedStoi(GF, Params.substr(0, Pos)) && GF > 0) { | ||
Params.erase(0, Pos + 1); | ||
MR = std::stoi(Params); | ||
// Factors are valid only if all parsed successfully: | ||
FactorsAreValid = GuardedStoi(MR, Params); | ||
// Note that MinRange = 0 is considered valid. | ||
} | ||
} | ||
ProcessedFactors = true; | ||
if (FactorsAreValid) { | ||
ProcessedFactors = true; | ||
} else { | ||
std::cerr | ||
<< "WARNING: Invalid value passed for " | ||
<< "SYCL_PARALLEL_FOR_RANGE_ROUNDING_PARAMS (Expected format " | ||
<< "MinRound:PreferredRound:MinRange, where MinRound, " | ||
"PreferredRound" | ||
<< " > 0, MinRange >= 0). Provided parameters will be ignored." | ||
<< std::endl; | ||
} | ||
} | ||
if (FactorsAreValid) { | ||
MinFactor = MF; | ||
GoodFactor = GF; | ||
MinRange = MR; | ||
} | ||
MinFactor = MF; | ||
GoodFactor = GF; | ||
MinRange = MR; | ||
} | ||
}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to remove
static
from the declaration of *RoundParams ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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