Stream: rfc-notifications

Topic: rfcs / issue #21 Cranelift/ISLE: allow extended left-hand...

view this post on Zulip RFC notifications bot (Apr 25 2022 at 04:54):

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!

view this post on Zulip RFC notifications bot (Apr 25 2022 at 11:06):

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.

view this post on Zulip RFC notifications bot (Apr 25 2022 at 16:55):

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, and emit, I think.

view this post on Zulip RFC notifications bot (Apr 25 2022 at 17:17):

cfallin commented on issue #21:

Ah, I forgot to cc @avanhatt re: verification concerns -- please let us know what you think as well!

view this post on Zulip RFC notifications bot (Apr 25 2022 at 17:17):

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

view this post on Zulip RFC notifications bot (Apr 25 2022 at 17:20):

fitzgen commented on issue #21:

(And that would let us get rid of optional expressions in given clauses, simplifying the language extension a little)

view this post on Zulip RFC notifications bot (Apr 25 2022 at 17:20):

fitzgen commented on issue #21:

Overall, I like this a lot!

view this post on Zulip RFC notifications bot (Apr 25 2022 at 17:21):

fitzgen commented on issue #21:

Are you imagining that pure external terms would only get &self context methods?

view this post on Zulip RFC notifications bot (Apr 25 2022 at 17:24):

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.

view this post on Zulip RFC notifications bot (Apr 25 2022 at 17:26):

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!

view this post on Zulip RFC notifications bot (Apr 26 2022 at 08:55):

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

view this post on Zulip RFC notifications bot (Apr 26 2022 at 16:14):

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 SSA Value; it is only needed once one decides to put the value in a register and use it in a machine instruction.

view this post on Zulip RFC notifications bot (Apr 26 2022 at 20:41):

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.

view this post on Zulip RFC notifications bot (Apr 27 2022 at 20:07):

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

Cornell

Fastly

IBM

Intel

Unaffiliated

view this post on Zulip RFC notifications bot (Apr 27 2022 at 20:17):

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

Cornell

Fastly

IBM

Intel

Unaffiliated

view this post on Zulip RFC notifications bot (Apr 27 2022 at 20:33):

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

Cornell

Fastly

IBM

Intel

Unaffiliated

view this post on Zulip RFC notifications bot (Apr 27 2022 at 20:56):

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

Cornell

Fastly

IBM

Intel

Unaffiliated

view this post on Zulip RFC notifications bot (Apr 27 2022 at 22:57):

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

Cornell

Fastly

IBM

Intel

Unaffiliated

view this post on Zulip RFC notifications bot (Apr 27 2022 at 22:59):

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

view this post on Zulip RFC notifications bot (Apr 28 2022 at 16:03):

uweigand commented on issue #21:

Looks good to me, thanks!

view this post on Zulip RFC notifications bot (Apr 28 2022 at 17:14):

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

Cornell

Fastly

IBM

Intel

Unaffiliated

view this post on Zulip RFC notifications bot (Apr 28 2022 at 17:14):

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

Cornell

Fastly

IBM

Intel

Unaffiliated

view this post on Zulip RFC notifications bot (Apr 28 2022 at 17:39):

sparker-arm commented on issue #21:

Yeah, LGTM too, thanks!

view this post on Zulip RFC notifications bot (Apr 28 2022 at 17:41):

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

Cornell

Fastly

IBM

Intel

Unaffiliated

view this post on Zulip RFC notifications bot (Apr 28 2022 at 18:35):

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

Cornell

Fastly

IBM

Intel

Unaffiliated

view this post on Zulip RFC notifications bot (Apr 28 2022 at 18:38):

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!

view this post on Zulip RFC notifications bot (Apr 29 2022 at 12:01):

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

Cornell

Fastly

IBM

Intel

Unaffiliated

Last updated: Nov 22 2024 at 17:03 UTC