Add a check that PredicateLoads must be used in the outermost split of a dimension #7788
Conversation
steven-johnson
added
the
release_notes
...except that the ones that would be affected weren't actually ever valid in the first place, since they may be subject to data overwriting. Breaking existing code in the name of fixing safety/correctness issue is always on the table, IMHO (and in this case, probably unavoidable). (Tagging with |
|
(I'm going to go ahead and test this inside google) |
That's why I double quoted "valid" : D |
|
Failures are unrelated, ready to land |
|
how does this affect the code inside Google? (if allowed to share at all lol) |
|
Not sure yet, need to run a test to find out :-) |
|
Testing in google looks good so far, stand by |
|
Testing in google showed zero failures, good to land |
| @@ -1090,6 +1090,36 @@ void Stage::split(const string &old, const string &outer, const string &inner, c | |||
| << "Use TailStrategy::GuardWithIf instead."; | |||
| } | |||
|
|
|||
| bool predicate_loads_ok = !exact; | |||
There was a problem hiding this comment.
I think PredicateLoads might actually be legal when exact is true. Let's discuss offline.
There was a problem hiding this comment.
No, exact is when you're splitting an rvar, which may scatter, so it's not legal to use PredicateLoads because it may scatter garbage values.
There was a problem hiding this comment.
which means we keep this line and the check?
There was a problem hiding this comment.
It depends which error message we want to trigger first. I guess we want the second one to trigger, because the first one says "you can't use PredicateLoads on this Var in this way", and the second says "you can't use PredicateLoads on this Var at all". So keep this line and the check.
There was a problem hiding this comment.
reflected, I think the PR is now ready to land
|
Failures seem unrelated, ready to land? |
…f a dimension (halide#7788) * add a check that PredicateLoads must be used in the outermost split of a dimension * newline * use the repro example * fix * avoid check for every other tail strategy * update error message to point out what's not allowed --------- Co-authored-by: Steven Johnson <srj@google.com>
Fixes #7764
From the above issue, we found out that using
PredicateLoadsin inner splits will write zeros to the tail region of the middle loop, but these tail regions overlap the valid region of the next outer loop iteration, so valid data may be overwritten with zeros.We can't prove the safety of using PredicateLoads in these cases, so in this PR we introduce a check that disallows such behavior.
PredicateLoadsmust be in the outermost split of a dimension.This could potentially be a big change since it makes the valid scheduling space smaller and may impact existing Halide programs ("valid" -> invalid). so cc @steven-johnson @abadams @derek-gerstmann