s390x: Add ISLE support

[uweigand]

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

[uweigand]

Just fixed a small merge conflict in lower.rs that would have introduces a regression. Should be good now.

[cfallin]

This is fantastic -- thank you for doing this migration so quickly!

One note below where I think we might want to do something slightly differently to make life easier for potential future testing/verification techniques -- but otherwise, I didn't see anything out-of-place and as long as the tests are passing, it looks good to me.

I like the ProducesBool abstraction for comparisons; perhaps we can adopt that in other backends as well.

cc @fitzgen if you want to look at any details here? Otherwise I'm fine with this merging (possibly after below comment addressed).

[cfallin]

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...)

[fitzgen]

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

[uweigand]

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.

[fitzgen]

I'm not really familiar with s390x but I gave this a read through and it all seems to make sense! LGTM modulo a couple nitpicks :)

[fitzgen]

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.

[uweigand]

Of course, done.

[fitzgen]

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

[fitzgen]

Along similar lines, I think this should be

Suggested change

	(rule (maybe_trap_if_sdiv_overflow _ _ _ _) (invalid_reg))
	(rule (maybe_trap_if_sdiv_overflow $false _ _ _) (invalid_reg))

[fitzgen]

Suggested change

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

[uweigand]

This version should address all issues discussed above.

[fitzgen]

LGTM! Thanks Ulrich!

@cfallin did you want to take another look, or should we go ahead and merge this?

[cfallin]

Was just looking at this as well; LGTM, happy for this to merge!