Stream: git-wasmtime

Topic: wasmtime / issue #10298 Cranelift: handle flags in a bett...

Wasmtime GitHub notifications bot (Feb 26 2025 at 17:43):

cfallin opened issue #10298:

As part of the discussion on #10276, the topic of our ProducesFlags/ConsumesFlags abstraction came up, and the general consensus is that although the type-safety it provides is welcome, the mechanism itself is clunky and annoying to maintain in several ways:

• Their existence implies that we need "raw" constructors for instructions that return the MInst directly rather than emitting them.
• We occasionally have to invent new enum arms for the producer and consumer side to encode different sorts of outputs (return just the producer's result Reg, just the consumer's, both as a pair, a pair of two consumers' results, etc.), and different lengths of sequences, and then update with_flags to handle the new cases.
• with_flags does not handle the full matrix of producer and consumer modes; we add lowering rules as we need them.

In general, it would be great if we could come up with a better solution. To recap, the main benefit that this abstraction provides is that it allows ISLE to ensure a flag producer and consumer (or multiple consumers) are placed next to each other in VCode, with no intermediate instruction clobbering flags. This is ordinarily not ensured in ISLE: we allow right-hand sides to encode a tree of operators and the emit order is implicit and not directly encoded (only that it obeys data dependencies, which is ensured by the ISLE evaluation order calling a constructor before its return value is used).

In the Cranelift weekly today we discussed a few alternatives:

• I recapped this comment's idea briefly: represent flags as a value, and then track which flags value is current during lowering, and panic if it is clobbered. In essence this still ensures correctness but does so by turning a correct-by-construction (typesafe) mechanism into a dynamic-at-compile-time check instead. It's simpler in some ways, but is it actually desirable? Consensus was no, we don't want this potential panic lurking beneath the surface -- that is another kind of developer unfriendliness.
• We could instead enumerate all N possible combinations of producers and consumers and make them pseudo-instructions. This provides the same static correct-by-construction guarantees that we have today, but without the clunkiness of the with_flags combinator: in essence we are replacing the proof-by-ISLE-types with one-time human inspection of the N sequences, given the hypothesis that N is small and that the human effort to verify once is trivial ("there are no instructions between producer and consumer"). Some concerns here are that N may not actually be very small if we have programmatic combination of producers and consumers from separate places, and this would require some refactoring to expand out the matrix.
• Finally, we could go one level lower in the problem space and address the "atomic sequence" need: provide a generic pseudoinstruction that some ISLE code can locally produce with the correct sequence, and then this instruction is atomically treated by the rest of the ISLE. The s390x backend already has something like this for some of its thread-atomics sequences; we could take inspiration from or reuse the mechanism in the other backends.

Consensus seemed to land on the last option. There is still some work to do to evaluate exactly how we can fit this into cases where ProducesFlags/ConsumesFlags values are passed programmatically between different helpers. E.g., IcmpCondResult is an interesting case -- do we just take a sequence there as the producer-sequence, and in e.g. jmp_cond_icmp, append the jump? This is still more safe than a carefully-crafted-emit-order solution, because all emits into the pseudoinst buffer are explicit; basically it's just admitting that we need to be more free-form about the sequences and less prescriptive about specific shapes of producer and consumer patterns. Also, the actual API for emitting instructions into a buffer needs to be carefully designed -- do we allow any inst to do so? Do we use the _raw constructors that folks mentioned over in #10276?

Wasmtime GitHub notifications bot (Feb 27 2025 at 17:57):

cfallin added the enhancement label to Issue #10298.

Wasmtime GitHub notifications bot (Feb 27 2025 at 17:57):

cfallin added the cranelift label to Issue #10298.

Last updated: Feb 28 2025 at 03:10 UTC