wasmtime / PR #3706 s390x: Add ISLE support · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #3706 s390x: Add ISLE support

Wasmtime GitHub notifications bot (Jan 20 2022 at 20:55):

uweigand opened PR #3706 from isle-s390x to main:

This adds ISLE support for the s390x back-end and moves lowering
of most instructions to ISLE. The only instructions still remaining
are calls, returns, traps, and branches, most of which will need
additional support in common code.

Generated code is not intended to be (significantly) differnet
than before; any additional optimizations due to the ISLE layer
can be added in follow-on patches. There were a few differences
in some filetests, but those are all just simple register allocation
changes (and all to the better!).

CC @cfallin @fitzgen

Wasmtime GitHub notifications bot (Jan 20 2022 at 21:45):

uweigand updated PR #3706 from isle-s390x to main.

Wasmtime GitHub notifications bot (Jan 20 2022 at 22:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jan 20 2022 at 22:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jan 20 2022 at 22:45):

cfallin created PR review comment:

A future-looking note here: when we get to verifying rules, in theory we want every rule to be independently correct, i.e., not only correct in cases not shadowed by other rules. Here and in some other spots nearby I notice the pattern of specific cases followed by wildcards, which is fine for this as long as the more general fallback is correct without the special case, but here appears not to be.

Earlier we had tried to follow the principle that rule ordering is completely flexible, and any applicable rule could fire, so the LHSes need to be disjoint or priorities need to be used if one must fire over the other. But I think that's pretty surprising and un-ergonomic in cases like this, so the DSL compiler now goes for "more specific rule first" which will indeed to the right thing here.

We'll undoubtedly uncover other cases like this if/when we do try to verify in this way but I just thought I'd note it as a principle we're trying to stick to elsewhere; here a (not_neg1) extractor or somesuch might cause fewer issues later...

(Part of me still likes the simplicity of "first written rule takes priority"; it's simple, explicit, no hidden magic... but others voiced good reasons not to do that, so, too late now probably...)

Wasmtime GitHub notifications bot (Jan 21 2022 at 00:45):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2022 at 00:45):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2022 at 00:45):

fitzgen created PR review comment:

Nitpick: it would be nice if the docs for this term and the other similar ones below it mentioned that this is a side-effectful operation.

Wasmtime GitHub notifications bot (Jan 21 2022 at 00:45):

fitzgen created PR review comment:

Along similar lines, I think this should be
(rule (maybe_trap_if_sdiv_overflow $false _ _ _) (invalid_reg))

Wasmtime GitHub notifications bot (Jan 21 2022 at 00:45):

fitzgen created PR review comment:

(rule (maybe_avoid_srem_overflow $false _ x _) x)

Wasmtime GitHub notifications bot (Jan 21 2022 at 00:45):

fitzgen created PR review comment:

We'll undoubtedly uncover other cases like this if/when we do try to verify in this way but I just thought I'd note it as a principle we're trying to stick to elsewhere; here a (not_neg1) extractor or somesuch might cause fewer issues later...

+1

Wasmtime GitHub notifications bot (Jan 21 2022 at 14:21):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2022 at 14:21):

uweigand created PR review comment:

OK, I wasn't aware of that requirement. For the divide checks, this should be straightforward using some extra extractors as you say. We still need a default case, but as long as the default is true instead of false it will still be always correct. However, I guess I then cannot use the (avoid_div_traps) extractor as-is, since I need the negated sense now. I don't think there's way to do that negation on ISLE, so I'd need another extractor e.g. like so:
(decl zero_divisor_check_needed (Value) bool)
(rule (zero_divisor_check_needed (u64_from_value (u64_nonzero _)) $false))
(rule (zero_divisor_check_needed (value_type (allow_div_traps)) $false))
(rule (zero_divisor_check_needed _ $true))
That new (allow_div_traps) should probably go into the common prelude as well.

However, I have made that same assumption extensively elsewhere, in particular when dispatching between different input types. For example, there's frequently the pattern of a pair of (fits_in_32 ty) and (fits_in_64 ty) rules, where 32-bit types must use the fits_in_32 rule, since the input isn't sufficiently sign- or zero-extended so the instruction behind the fits_in_64 rule would yield an incorrect result. Similarly, for e.g. moves I have both a (fits_in_32 ty) rule and a $F32 rule, where the fits_in_32 rule is incorrect for $F32 as it uses a GPR instruction.

To ensure each rule is independently correct (if not optimal), I need to rework all this type handling. This probably means making supported types for each rule much more explicit, which is probably a good thing in the long run anyway.

I'll be working on this and provide an updated patch.

Wasmtime GitHub notifications bot (Jan 21 2022 at 14:23):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2022 at 14:23):

uweigand created PR review comment:

Of course, done.

Wasmtime GitHub notifications bot (Jan 21 2022 at 18:32):

uweigand updated PR #3706 from isle-s390x to main.

Wasmtime GitHub notifications bot (Jan 21 2022 at 19:22):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2022 at 19:25):

cfallin merged PR #3706.

Last updated: Nov 22 2024 at 17:03 UTC