Wasmtime GitHub notifications bot (Apr 23 2020 at 17:37): Wasmtime GitHub notifications bot (Apr 23 2020 at 17:37):bnjbvr opened PR #1586 from address-sp-add to master:
- use explicit address lowering for passing stack arguments to calls, when the offset is large.
One question is: what
Instshould cause a move from SP to another register? I preferred to keep the "high-level" semantics ofInst::Mov, instead of relying on every producer to produce anInst::Add { rd, sp, 0 }. This means we have to modify the codegen ofInst::Movwhen SP is involved, but it seemed like fine blackboxing.The PR as a whole is a bit messy. I was wondering if
gen_copy_reg_to_argshouldn't just take thectxargument, but then type parameters were involved, making the trait more contrived. I didn't look into it, it might be the right thing to do in the future (otherwise this aarch64 impl detail leaks into the general interface, which is bad).Good thing is that thanks to many new assertions, error like this one should be more easily caught in the future.
cc @julian-seward1 because I was hitting an assertion in regalloc if I didn't include the change in
is_move:assert!(iix_bounds.uses_len == 1);(backtracking.rs:572). That happens during coalescing; I checked and the value was 0, instead of the expected 1. I don't think the change inis_moveought to be correct, so it'd be nice to see if there's something else to do to fix this.
Wasmtime GitHub notifications bot (Apr 23 2020 at 17:37):bnjbvr requested cfallin for a review on PR #1586.
Wasmtime GitHub notifications bot (Apr 23 2020 at 17:38):bnjbvr edited PR #1586 from address-sp-add to master:
and use explicit address lowering for passing stack arguments to calls, when the offset is large.
One question is: what
Instshould cause a move from SP to another register? I preferred to keep the "high-level" semantics ofInst::Mov, instead of relying on every producer to produce anInst::Add { rd, sp, 0 }. This means we have to modify the codegen ofInst::Movwhen SP is involved, but it seemed like fine blackboxing.The PR as a whole is a bit messy. I was wondering if
gen_copy_reg_to_argshouldn't just take thectxargument, but then type parameters were involved, making the trait more contrived. I didn't look into it, it might be the right thing to do in the future (otherwise this aarch64 impl detail leaks into the general interface, which is bad).Good thing is that thanks to many new assertions, error like this one should be more easily caught in the future.
cc @julian-seward1 because I was hitting an assertion in regalloc if I didn't include the change in
is_move:assert!(iix_bounds.uses_len == 1);(backtracking.rs:572). That happens during coalescing; I checked and the value was 0, instead of the expected 1. I don't think the change inis_moveought to be correct, so it'd be nice to see if there's something else to do to fix this.
Wasmtime GitHub notifications bot (Apr 23 2020 at 19:12):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Apr 23 2020 at 19:12):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Apr 23 2020 at 19:12):cfallin created PR Review Comment:
Add a FIXME comment referring to a regalloc issue number, maybe?
Wasmtime GitHub notifications bot (Apr 23 2020 at 19:12):cfallin created PR Review Comment:
Hmm. I don't want to block this PR on it, but I'm thinking we should really clean up the ABI traits to take a
LowerCtxand just emit instructions and allocate temps as needed directly, as you suggested above. @alexcrichton hit a similar issue with is stack-check PR (#1573). It's my design flaw originally (sorry!) so I can take this as a followup item once that PR and this one land.
Wasmtime GitHub notifications bot (Apr 23 2020 at 19:48):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Apr 23 2020 at 19:48):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Apr 23 2020 at 19:48):bjorn3 created PR Review Comment:
What is the difference between this and
store_stack_sp?
Wasmtime GitHub notifications bot (Apr 23 2020 at 19:49):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Apr 23 2020 at 19:49):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Apr 23 2020 at 19:49):bjorn3 created PR Review Comment:
Other archs may not need this.
Wasmtime GitHub notifications bot (Apr 24 2020 at 09:30):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Apr 24 2020 at 09:30):bnjbvr created PR Review Comment:
store_stack_spmay generate several instructions to lower the SP addition.
Wasmtime GitHub notifications bot (Apr 24 2020 at 09:30):bnjbvr created PR Review Comment:
Yes, see also other comments.
Wasmtime GitHub notifications bot (Apr 24 2020 at 09:30):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Apr 24 2020 at 10:42):bnjbvr updated PR #1586 from address-sp-add to master:
and use explicit address lowering for passing stack arguments to calls, when the offset is large.
One question is: what
Instshould cause a move from SP to another register? I preferred to keep the "high-level" semantics ofInst::Mov, instead of relying on every producer to produce anInst::Add { rd, sp, 0 }. This means we have to modify the codegen ofInst::Movwhen SP is involved, but it seemed like fine blackboxing.The PR as a whole is a bit messy. I was wondering if
gen_copy_reg_to_argshouldn't just take thectxargument, but then type parameters were involved, making the trait more contrived. I didn't look into it, it might be the right thing to do in the future (otherwise this aarch64 impl detail leaks into the general interface, which is bad).Good thing is that thanks to many new assertions, error like this one should be more easily caught in the future.
cc @julian-seward1 because I was hitting an assertion in regalloc if I didn't include the change in
is_move:assert!(iix_bounds.uses_len == 1);(backtracking.rs:572). That happens during coalescing; I checked and the value was 0, instead of the expected 1. I don't think the change inis_moveought to be correct, so it'd be nice to see if there's something else to do to fix this.
Wasmtime GitHub notifications bot (Apr 24 2020 at 15:38):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Apr 24 2020 at 15:38):bnjbvr created PR Review Comment:
I looked into this, and I could make it work as discussed, but it puts the type parameter down to the implementation of
store_stack_sp, which is unfortunate. Maybe we could revisit this later, and instead manually monomorphize theLowerCtxtrait into its own implLower.
Wasmtime GitHub notifications bot (Apr 24 2020 at 15:38):bnjbvr updated PR #1586 from address-sp-add to master:
and use explicit address lowering for passing stack arguments to calls, when the offset is large.
One question is: what
Instshould cause a move from SP to another register? I preferred to keep the "high-level" semantics ofInst::Mov, instead of relying on every producer to produce anInst::Add { rd, sp, 0 }. This means we have to modify the codegen ofInst::Movwhen SP is involved, but it seemed like fine blackboxing.The PR as a whole is a bit messy. I was wondering if
gen_copy_reg_to_argshouldn't just take thectxargument, but then type parameters were involved, making the trait more contrived. I didn't look into it, it might be the right thing to do in the future (otherwise this aarch64 impl detail leaks into the general interface, which is bad).Good thing is that thanks to many new assertions, error like this one should be more easily caught in the future.
cc @julian-seward1 because I was hitting an assertion in regalloc if I didn't include the change in
is_move:assert!(iix_bounds.uses_len == 1);(backtracking.rs:572). That happens during coalescing; I checked and the value was 0, instead of the expected 1. I don't think the change inis_moveought to be correct, so it'd be nice to see if there's something else to do to fix this.
Wasmtime GitHub notifications bot (Apr 24 2020 at 15:41):bnjbvr updated PR #1586 from address-sp-add to master:
and use explicit address lowering for passing stack arguments to calls, when the offset is large.
One question is: what
Instshould cause a move from SP to another register? I preferred to keep the "high-level" semantics ofInst::Mov, instead of relying on every producer to produce anInst::Add { rd, sp, 0 }. This means we have to modify the codegen ofInst::Movwhen SP is involved, but it seemed like fine blackboxing.The PR as a whole is a bit messy. I was wondering if
gen_copy_reg_to_argshouldn't just take thectxargument, but then type parameters were involved, making the trait more contrived. I didn't look into it, it might be the right thing to do in the future (otherwise this aarch64 impl detail leaks into the general interface, which is bad).Good thing is that thanks to many new assertions, error like this one should be more easily caught in the future.
cc @julian-seward1 because I was hitting an assertion in regalloc if I didn't include the change in
is_move:assert!(iix_bounds.uses_len == 1);(backtracking.rs:572). That happens during coalescing; I checked and the value was 0, instead of the expected 1. I don't think the change inis_moveought to be correct, so it'd be nice to see if there's something else to do to fix this.
Wasmtime GitHub notifications bot (Apr 24 2020 at 17:30):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Apr 24 2020 at 17:30):cfallin created PR Review Comment:
Hmm, indeed; I want to try to keep things parameterized on the
Lowertrait (and not simplify to just the one impl) because it gives us flexibility later to e.g. be generic over multiple input IRs. This seems fine for now, I think.
Wasmtime GitHub notifications bot (Apr 24 2020 at 17:32):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Apr 24 2020 at 17:33):cfallin merged PR #1586.
Last updated: Dec 23 2024 at 13:07 UTC