cfallin commented on issue #21:
This is an elaboration of some of the high-level thoughts I mentioned in the last Cranelift biweekly, and originally came from some discussions and head-scratching while pair-implementing some x64 rules with @abrown (thanks!). I spent some time playing with a bunch of different corners of the design space and this seems like the best option so far, but I'm curious if anyone has better ideas; I figured this was at the point that I could lay it out for feedback, in any case!
sparker-arm commented on issue #21:
This looks good to me... I'm currently trying to port IaddPairwise lowering to {U|S}ADDLP for aarch64 and I'm not finding it intuitive, so anything that makes it clearer how to use a N-to-1 extractor(s) would be great. My specific problem requires pattern matching a diamond shaped DAG and, I think, from your examples of current hacks, I can currently achieve this with using an approach from @uweigand. But, again, it doesn't look natural or really make obvious sense to me.
For my clarification, can current extractors use put_in_reg? Or is this considered as having side-effects?
I also like having the 'given' for predicates, such as for architecture features.
cfallin commented on issue #21:
Thanks! For what it's worth @sparker-arm I'd be happy to help you work out how to get particular lowerings to work sometime, if you think that would be useful. (I've done some Zoom pair-implementing with abrown before and it's always interesting and educational for both of us!)
For my clarification, can current extractors use put_in_reg? Or is this considered as having side-effects?
It's a side-effecting constructor, so this would only come in the RHS. In general the phasing divide that makes sense I think is to collect as much as we need to match in the LHS (this includes "pure" parts that we may or may not be able to build, like certain immediate kinds), and then start the emission actions in the RHS. The only kinds of actions that have to come in the RHS (after we commit) are
put_in_reg
,sink_inst
, andemit
, I think.
cfallin commented on issue #21:
Ah, I forgot to cc @avanhatt re: verification concerns -- please let us know what you think as well!
fitzgen commented on issue #21:
This example
(rule (lower (magic_simd_op x y)) (given (magic_simd_extension_enabled)) (x64_magic_simd_op x y))
seems like it would be better written as the following so that it isn't "backwards" -- which users have found confusing -- with the new language extension:
(rule (lower (magic_simd_op x y)) (given _ (magic_simd_extension_enabled)) (x64_magic_simd_op x y))
fitzgen commented on issue #21:
(And that would let us get rid of optional expressions in
given
clauses, simplifying the language extension a little)
fitzgen commented on issue #21:
Overall, I like this a lot!
fitzgen commented on issue #21:
Are you imagining that pure external terms would only get
&self
context methods?
cfallin commented on issue #21:
Are you imagining that pure external terms would only get
&self
context methods?I actually hadn't considered that at least at first, as extractors take
&mut self
right now as well (if I remember right this was mostly for consistency but there may be some cases where we memoize as well). But it does seem like a nice long-term goal to take&self
both for extractors and pure constructors, for more safety in the Rust bindings.
cfallin commented on issue #21:
(And that would let us get rid of optional expressions in
given
clauses, simplifying the language extension a little)Hmm, yeah, I like your version better here, thanks!
sparker-arm commented on issue #21:
I'd be happy to help you work out how to get particular lowerings to work sometime
Thanks!
It's a side-effecting constructor, so this would only come in the RHS.
Sorry to go off track, but why is it that to look at a value produced by an instruction, we have to put_in_reg first? Can't extractors understand an SSA value from an instruction, without causing side-effects? As a concrete example, is something like a simple optimisation like this possible?
(rule (lower (isub x y)) (given (are_equal x y)) (iconst 0))
cfallin commented on issue #21:
Sorry to go off track, but why is it that to look at a value produced by an instruction, we have to put_in_reg first? Can't extractors understand an SSA value from an instruction, without causing side-effects? As a concrete example, is something like a simple optimisation like this possible?
(rule (lower (isub x y)) (given (are_equal x y)) (iconst 0))
That's definitely possible!
put_in_reg
isn't needed to look at the SSAValue
; it is only needed once one decides to put the value in a register and use it in a machine instruction.
cfallin commented on issue #21:
Thanks all -- updated based on feedback!
It seems like there are no really big concerns here yet (and after speaking with @avanhatt, @mlfbrown and @mpardesh today it sounds like this should work for
isle-veri
as well), but please let me know if there are any other lingering concerns. Otherwise I'll likely move this to final-call status in a day or two.
cfallin commented on issue #21:
Alright, it is now time for the FCP, I think! I'll create the stakeholders list below based on committers to Cranelift active in the past three months (since Jan 26, anything more than a trivial doc fix) and Cranelift-related RFCs, plus anyone who has commented here.
Motion to finalize with a disposition to merge
Stakeholders sign-off
Arm
- [ ] @akirilov-arm
- [ ] @sparker-arm
- [ ] @dheaton-arm
- [ ] @FreddieLiardet
Cornell
- [ ] @avanhatt
Fastly
- [ ] @alexcrichton
- [x] @cfallin
- [ ] @fitzgen
IBM
- [ ] @uweigand
Intel
- [ ] @abrown
- [ ] @jlb6740
Unaffiliated
- [ ] @bjorn3
fitzgen edited a comment on issue #21:
Alright, it is now time for the FCP, I think! I'll create the stakeholders list below based on committers to Cranelift active in the past three months (since Jan 26, anything more than a trivial doc fix) and Cranelift-related RFCs, plus anyone who has commented here.
Motion to finalize with a disposition to merge
Stakeholders sign-off
Arm
- [ ] @akirilov-arm
- [ ] @sparker-arm
- [ ] @dheaton-arm
- [ ] @FreddieLiardet
Cornell
- [ ] @avanhatt
Fastly
- [ ] @alexcrichton
- [x] @cfallin
- [x] @fitzgen
IBM
- [ ] @uweigand
Intel
- [ ] @abrown
- [ ] @jlb6740
Unaffiliated
- [ ] @bjorn3
alexcrichton edited a comment on issue #21:
Alright, it is now time for the FCP, I think! I'll create the stakeholders list below based on committers to Cranelift active in the past three months (since Jan 26, anything more than a trivial doc fix) and Cranelift-related RFCs, plus anyone who has commented here.
Motion to finalize with a disposition to merge
Stakeholders sign-off
Arm
- [ ] @akirilov-arm
- [ ] @sparker-arm
- [ ] @dheaton-arm
- [ ] @FreddieLiardet
Cornell
- [ ] @avanhatt
Fastly
- [x] @alexcrichton
- [x] @cfallin
- [x] @fitzgen
IBM
- [ ] @uweigand
Intel
- [ ] @abrown
- [ ] @jlb6740
Unaffiliated
- [ ] @bjorn3
cfallin edited a comment on issue #21:
Alright, it is now time to make a motion to finalize, I think! I'll create the stakeholders list below based on committers to Cranelift active in the past three months (since Jan 26, anything more than a trivial doc fix) and Cranelift-related RFCs, plus anyone who has commented here.
Motion to finalize with a disposition to merge
Stakeholders sign-off
Arm
- [ ] @akirilov-arm
- [ ] @sparker-arm
- [ ] @dheaton-arm
- [ ] @FreddieLiardet
Cornell
- [ ] @avanhatt
Fastly
- [x] @alexcrichton
- [x] @cfallin
- [x] @fitzgen
IBM
- [ ] @uweigand
Intel
- [ ] @abrown
- [ ] @jlb6740
Unaffiliated
- [ ] @bjorn3
cfallin edited a comment on issue #21:
Alright, it is now time to make a motion to finalize, I think! I'll create the stakeholders list below based on committers to Cranelift active in the past three months (since Jan 26, anything more than a trivial doc fix) and Cranelift-related RFCs, plus anyone who has commented here.
Motion to finalize with a disposition to merge
Stakeholders sign-off
Arm
- [ ] @akirilov-arm
- [ ] @sparker-arm
- [ ] @dheaton-arm
- [ ] @FreddieLiardet
Cornell
- [ ] @avanhatt
Fastly
- [x] @alexcrichton
- [x] @cfallin
- [x] @fitzgen
IBM
- [ ] @uweigand
Intel
- [x] @abrown
- [ ] @jlb6740
Unaffiliated
- [ ] @bjorn3
cfallin commented on issue #21:
With approvals from two separate groups (Fastly and Intel) this now enters the Final Comment Period, which will end on Mon May 9 (unless all groups have at least one approval, shortcutting the process, or any objections are raised, pausing it).
uweigand commented on issue #21:
Looks good to me, thanks!
cfallin edited a comment on issue #21:
Alright, it is now time to make a motion to finalize, I think! I'll create the stakeholders list below based on committers to Cranelift active in the past three months (since Jan 26, anything more than a trivial doc fix) and Cranelift-related RFCs, plus anyone who has commented here.
Motion to finalize with a disposition to merge
Stakeholders sign-off
Arm
- [ ] @akirilov-arm
- [ ] @sparker-arm
- [ ] @dheaton-arm
- [ ] @FreddieLiardet
Cornell
- [ ] @avanhatt
Fastly
- [x] @alexcrichton
- [x] @cfallin
- [x] @fitzgen
IBM
- [x] @uweigand
Intel
- [x] @abrown
- [ ] @jlb6740
Unaffiliated
- [ ] @bjorn3
cfallin edited a comment on issue #21:
Alright, it is now time to make a motion to finalize, I think! I'll create the stakeholders list below based on committers to Cranelift active in the past three months (since Jan 26, anything more than a trivial doc fix) and Cranelift-related RFCs, plus anyone who has commented here.
Motion to finalize with a disposition to merge
Stakeholders sign-off
Arm
- [ ] @akirilov-arm
- [ ] @sparker-arm
- [ ] @dheaton-arm
- [ ] @FreddieLiardet
Cornell
- [x] @avanhatt
Fastly
- [x] @alexcrichton
- [x] @cfallin
- [x] @fitzgen
IBM
- [x] @uweigand
Intel
- [x] @abrown
- [ ] @jlb6740
Unaffiliated
- [ ] @bjorn3
sparker-arm commented on issue #21:
Yeah, LGTM too, thanks!
cfallin edited a comment on issue #21:
Alright, it is now time to make a motion to finalize, I think! I'll create the stakeholders list below based on committers to Cranelift active in the past three months (since Jan 26, anything more than a trivial doc fix) and Cranelift-related RFCs, plus anyone who has commented here.
Motion to finalize with a disposition to merge
Stakeholders sign-off
Arm
- [ ] @akirilov-arm
- [x] @sparker-arm
- [ ] @dheaton-arm
- [ ] @FreddieLiardet
Cornell
- [x] @avanhatt
Fastly
- [x] @alexcrichton
- [x] @cfallin
- [x] @fitzgen
IBM
- [x] @uweigand
Intel
- [x] @abrown
- [ ] @jlb6740
Unaffiliated
- [ ] @bjorn3
cfallin edited a comment on issue #21:
Alright, it is now time to make a motion to finalize, I think! I'll create the stakeholders list below based on committers to Cranelift active in the past three months (since Jan 26, anything more than a trivial doc fix) and Cranelift-related RFCs, plus anyone who has commented here.
Motion to finalize with a disposition to merge
Stakeholders sign-off
Arm
- [ ] @akirilov-arm
- [x] @sparker-arm
- [ ] @dheaton-arm
- [ ] @FreddieLiardet
Cornell
- [x] @avanhatt
Fastly
- [x] @alexcrichton
- [x] @cfallin
- [x] @fitzgen
IBM
- [x] @uweigand
Intel
- [x] @abrown
- [ ] @jlb6740
Unaffiliated
- [x] @bjorn3
cfallin commented on issue #21:
With approvals from all stakeholder groups above, the final comment period has ended early and this RFC will merge: as per the RFC process,
Finally, the RFC is automatically merged/close if either: * The FCP elapses without any objections. * A stakeholder from each group has signed off, short-cutting the waiting period.
Thanks all! I'll work on cleaning up the draft implementation in bytecodealliance/wasmtime#4072, incorporating changes from here (notably
given
->if-let
), and hopefully we can start using this in our lowering rules soon!
akirilov-arm edited a comment on issue #21:
Alright, it is now time to make a motion to finalize, I think! I'll create the stakeholders list below based on committers to Cranelift active in the past three months (since Jan 26, anything more than a trivial doc fix) and Cranelift-related RFCs, plus anyone who has commented here.
Motion to finalize with a disposition to merge
Stakeholders sign-off
Arm
- [x] @akirilov-arm
- [x] @sparker-arm
- [ ] @dheaton-arm
- [ ] @FreddieLiardet
Cornell
- [x] @avanhatt
Fastly
- [x] @alexcrichton
- [x] @cfallin
- [x] @fitzgen
IBM
- [x] @uweigand
Intel
- [x] @abrown
- [ ] @jlb6740
Unaffiliated
- [x] @bjorn3
Last updated: Dec 23 2024 at 12:05 UTC