Add TailStrategy::Predicate #5856
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abadams
reviewed
Mar 26, 2021
|
The existence of a static Call::as_tag that takes an Expr sort of implies that Tags should be their own IR nodes rather than a call type. What do you think the pros/cons of making Tag a separate IRNode are? E.g. would it duplicate a bunch of code from Call handlers and methods? |
|
I don't know that Call::as_tag implies tags should be their own IR node. I do think that might make sense regardless. Pros:
Cons:
I think that con is pretty big, and the pros are pretty small. At the very least, we should make that change in a separate PR I think. |
abadams
reviewed
Mar 30, 2021
| if (call && (call->is_intrinsic(Call::likely) || | ||
| call->is_intrinsic(Call::likely_if_innermost) || | ||
| call->is_intrinsic(Call::strict_float))) { | ||
| const Call *call = Call::as_tag(c); |
There was a problem hiding this comment.
if (const Call *call == Call::as_tag(c)) {
There was a problem hiding this comment.
call is used below :) I made this change at first and had to revert it for that.
abadams
reviewed
Mar 30, 2021
abadams
reviewed
Mar 30, 2021
abadams
approved these changes
Mar 30, 2021
steven-johnson
added a commit
that referenced
this pull request
Mar 31, 2021
Bug injected in #5856: the change in Simplify_Let.cpp was inadvertently stripping `strict_float()` calls that wrapped the RHS of a Let-expr, which can change results nontrivially in some cases. I don't think a new test for this fix is practical -- it would be a little fragile, as it would rely on the specifics of simplification that could change over time. As a drive-by, also added an explicit rule to Simplify_Call to ensure that strict_float(strict_float(x)) -> strict_float(x) in *all* cases. (The existing rule didn't do this in all cases.) Also added an assertion to Codegen_LLVM.cpp that no calls of the form strict_float(strict_float(x)) should ever be seen at that point.
steven-johnson
added a commit
that referenced
this pull request
Mar 31, 2021
* Don't strip strict_float() from lets Bug injected in #5856: the change in Simplify_Let.cpp was inadvertently stripping `strict_float()` calls that wrapped the RHS of a Let-expr, which can change results nontrivially in some cases. I don't think a new test for this fix is practical -- it would be a little fragile, as it would rely on the specifics of simplification that could change over time. As a drive-by, also added an explicit rule to Simplify_Call to ensure that strict_float(strict_float(x)) -> strict_float(x) in *all* cases. (The existing rule didn't do this in all cases.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a new
TailStrategy::Predicate. This is very similar toTailStrategy::GuardWithIf, except it uses predicated loops unconditionally, instead of heuristically choosing based on the target.On some targets, we think using predicated loads/stores is always faster than scalarizing (Hexagon). On these targets, we can just use predicated loads/stores for
TailStrategy::GuardWithIf. On these targets, usingTailStrategy::Predicatewill not change anything.On most other targets, predicated loads/stores are expensive (emulated with scalar code). In particular, the cost of inserting and extracting the scalars to/from vectors can be high. On these targets, Halide scalarizes the whole body of the loop instead. This is unfortunate in some cases, when the loop is very expensive, it can be worth the added overhead to assemble the scalarized loads/stores into vectors, so we can vectorize the expensive computation. This PR allows programmers to ask for this with the new tail strategy.