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
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
uweigand updated PR #3706 from isle-s390x
to main
.
cfallin submitted PR review.
cfallin submitted PR review.
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...)
fitzgen submitted PR review.
fitzgen submitted PR review.
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.
fitzgen created PR review comment:
Along similar lines, I think this should be
(rule (maybe_trap_if_sdiv_overflow $false _ _ _) (invalid_reg))
fitzgen created PR review comment:
(rule (maybe_avoid_srem_overflow $false _ x _) x)
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
uweigand submitted PR review.
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 offalse
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 thefits_in_32
rule, since the input isn't sufficiently sign- or zero-extended so the instruction behind thefits_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 thefits_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.
uweigand submitted PR review.
uweigand created PR review comment:
Of course, done.
uweigand updated PR #3706 from isle-s390x
to main
.
fitzgen submitted PR review.
cfallin merged PR #3706.
Last updated: Dec 23 2024 at 12:05 UTC