Cranelift: add opportunistic value defs, use for uadd_overflow + brif folds. - #14228
Cranelift: add opportunistic value defs, use for uadd_overflow + brif folds.#14228cfallin wants to merge 3 commits into
uadd_overflow + brif folds.#14228Conversation
…rif` folds Add a new lowering primitive, an "opportunistic def", which allows a lowering that incidentally computes another value to register that value for possible later use. When the backward scan reaches the value's actual definition, if the use-count has not grown (no further uses appeared while scanning up), the definition can be skipped entirely and the value aliased to the opportunistically-computed regs. Also add a `value_used` helper that reports whether a value still has uses, allowing skipping of some part of a lowering when not needed. With these two features available in the lowering environment, this PR then adds new lowerings for brif-of-`uadd_overflow`, and bare `uadd_overflow`, on x86-64 and aarch64: - When `uadd_overflow`'s overflow flag is used as a branch condition, the branch directly uses flags produced by the `add` instruction, skipping (slow and verbose) materialization of the bool overflow flag into a GPR. If the sum is only used past the branch, then this `add` also produces that value; so only one `add` is ever emitted. Thus a `add; SETcc; test; jnz` sequence turns into (x64) `add; jb` / (aarch64) `adds; b.hs`. If the sum is used below the `uadd_overflow` but above the branch, another `add` is *also* emitted. That's fine: adds are cheap; cheaper certainly than materializing flags with `SETcc` or `CSET`. But to optimize that case further... - ...when `uadd_overflow` is *only* used for its sum, and not its overflow flag, we now emit an `add` without a `SETcc`. As a result of these lowerings, the case `v1, v2 = uadd_overflow ...; brif v2, ...` lowers to `add; jb`, and the case `v1, v2 = uadd_overflow ...; (use v1); brif v2, ...` lowers to `add; (use sum); add; jb`. The *only* remaining case where we use a slow `SETcc` is when the bool result is actually materialized and used as an integer value; or if the add and branch are pushed into separate blocks. This resolves the same issue as bytecodealliance#13919, following discussion in that PR and the Cranelift weekly meeting. In particular, (i) we do not build an ad-hoc separate scan (this mechanism works as part of the main lowering/instruction-selection scan); and (ii) we are resilient to instructions placed between the `uadd_overflow` and branch, which is likely to happen due to egraph demand-based elaboration, and which foils a simpler peephole-based approach.
|
(This has been on my TODO list for a long time; sorry about the delay, @playX18, but hopefully this addresses your use-case!) |
alexcrichton
left a comment
There was a problem hiding this comment.
Seems reasonable to me! Question on this: would it be possible to somehow determine, before fusing this into a branch, if the opportunistic def is going to be thrown away and re-calculated? For something like an addition it seems probably fine to always duplicate that, but for something like a multiplication it might be better to never duplicate that given its latency. (maybe? unsure). Basically it seems to me like a reasonable heuristic here would be to only fuse into branches where the opportunistic def actually works as the def, and in all other cases fall back to materializing the flag and then testing it later.
Although now that I actually write this down what I'm going for is to put the onus on CLIF producers to make sure the flag-and-branch are close together such that the opportunistic def always matches. In some sense that's no different from this PR as-is where it's still on them to do that to avoid the double-translate if it matters... Anyway, still curious on the question at least as a data point
| (second_result | ||
| uadd_inst @ (uadd_overflow (ty_32_or_64 ty) x y)) _ _) (two_targets taken not_taken)) | ||
| (if-let (first_result v1) uadd_inst) | ||
| (let ((producer ProducesFlags (alu_rrr_with_flags_paired ty x y (ALUOp.AddS))) | ||
| (sum Reg (produces_flags_get_reg producer)) | ||
| (_ Unit (opportunistic_def v1 sum))) |
There was a problem hiding this comment.
Would it be possible to fold this into is_nonzero_cmp instead of having a rule specifically for brif? That'd then additionally handle the condition flowing intp a trapz, trapnz, and select all at the same time. If this does work, could this be done for x64 too?
There was a problem hiding this comment.
Additionally, since we might want to use this for a number of other things, could a new helper like produces_flags_with_opportunistic_def be added? That'd internally then use produces_flags_get_reg, call opportunistic_def, and then return a ProducesFlags created with produces_flags_ignore. Basically factoring out the boilerplate-y parts of this if possible
There was a problem hiding this comment.
I played with this a bit, but IMHO it's not really that much simpler to fold this into ProducesFlags. It requires pulling apart all the pieces from the original inst anyway and stuffing them into the ProducesFlags (the other result value, the emitted result from the adds) and requires thinking about the emission combinations with various ConsumesFlags. Arguably more brittle, and distributes the logic over a larger area. In other words I don't think it actually avoids any boilerplate, it just explodes it into shards of boiler iron embedded in the walls...
There was a problem hiding this comment.
(Happy to tackle this or see someone else tackle this in a followup of course if there are more ideas here -- but maybe good to get the building blocks in first regardless?)
Unfortunately we can't really know "the future" during the backward lowering scan (or, well, we could, but that would be a separate scan). The conclusion I came to is that it's way simpler to build this opportunistic mechanism and just let the lowering fire twice if there is actually a user in the block in between the add and branch. I also address this with "If the sum is used below the uadd_overflow but above the branch, another add is also emitted. That's fine: adds are cheap; cheaper certainly than materializing flags with SETcc or CSET.", I think: what we get here is actually still cheaper than what you're asking for (strictly one fewer instruction -- |
Add a new lowering primitive, an "opportunistic def", which allows a lowering that incidentally computes another value to register that value for possible later use. When the backward scan reaches the value's actual definition, if the use-count has not grown (no further uses appeared while scanning up), the definition can be skipped entirely and the value aliased to the opportunistically-computed regs.
Also add a
value_usedhelper that reports whether a value still has uses, allowing skipping of some part of a lowering when not needed.With these two features available in the lowering environment, this PR then adds new lowerings for brif-of-
uadd_overflow, and bareuadd_overflow, on x86-64 and aarch64:When
uadd_overflow's overflow flag is used as a branch condition, the branch directly uses flags produced by theaddinstruction, skipping (slow and verbose) materialization of the bool overflow flag into a GPR.If the sum is only used past the branch, then this
addalso produces that value; so only oneaddis ever emitted. Thus aadd; SETcc; test; jnzsequence turns into (x64)add; jb/ (aarch64)adds; b.hs.If the sum is used below the
uadd_overflowbut above the branch, anotheraddis also emitted. That's fine: adds are cheap; cheaper certainly than materializing flags withSETccorCSET.But to optimize that case further...
...when
uadd_overflowis only used for its sum, and not its overflow flag, we now emit anaddwithout aSETcc.As a result of these lowerings, the case
v1, v2 = uadd_overflow ...; brif v2, ...lowers toadd; jb, and the casev1, v2 = uadd_overflow ...; (use v1); brif v2, ...lowers toadd; (use sum); add; jb. The only remaining case where we use a slowSETccis when the bool result is actually materialized and used as an integer value; or if the add and branch are pushed into separate blocks.This resolves the same issue as #13919, following discussion in that PR and the Cranelift weekly meeting. In particular, (i) we do not build an ad-hoc separate scan (this mechanism works as part of the main lowering/instruction-selection scan); and (ii) we are resilient to instructions placed between the
uadd_overflowand branch, which is likely to happen due to egraph demand-based elaboration, and which foils a simpler peephole-based approach.