Stream: git-cranelift

Topic: cranelift / PR #1304 Encode stack load store

GitHub (Dec 20 2019 at 20:18):

bjorn3 opened PR #1304 from encode_stack_load_store to master:

• [ ] 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

This currently doesn't encode stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00

GitHub (Dec 20 2019 at 20:19):

bjorn3 edited PR #1304 from encode_stack_load_store to master:

• [ ] 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

This currently doesn't encode stack_load and stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00

GitHub (Dec 22 2019 at 16:14):

bjorn3 updated PR #1304 from encode_stack_load_store to master:

• [ ] 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

This currently doesn't encode stack_load and stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00

GitHub (Jan 04 2020 at 11:43):

bjorn3 edited PR #1304 from encode_stack_load_store to master:

• [x] This has been discussed in issue #597, 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

This currently doesn't encode stack_load and stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00

GitHub (Jan 25 2020 at 12:46):

bjorn3 updated PR #1304 from encode_stack_load_store to master:

• [x] This has been discussed in issue #597, 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

This currently doesn't encode stack_load and stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00

GitHub (Jan 25 2020 at 12:47):

bjorn3 updated PR #1304 from encode_stack_load_store to master:

• [x] This has been discussed in issue #597, 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

This currently doesn't encode stack_load and stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00

GitHub (Jan 25 2020 at 13:57):

bjorn3 updated PR #1304 from encode_stack_load_store to master:

• [x] This has been discussed in issue #597, 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

This currently doesn't encode stack_load and stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00

GitHub (Jan 25 2020 at 13:58):

bjorn3 updated PR #1304 from encode_stack_load_store to master:

• [x] This has been discussed in issue #597, 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

This currently doesn't encode stack_load and stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00

GitHub (Jan 25 2020 at 14:05):

bjorn3 updated PR #1304 from encode_stack_load_store to master:

• [x] This has been discussed in issue #597, 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

This currently doesn't encode stack_load and stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00

GitHub (Jan 25 2020 at 14:08):

bjorn3 edited PR #1304 from encode_stack_load_store to master:

• [x] This has been discussed in issue #597, 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.

• [x] 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

This currently doesn't encode stack_load and stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00

GitHub (Jan 25 2020 at 14:10):

bjorn3 edited PR #1304 from encode_stack_load_store to master:

• [x] This has been discussed in issue #597, or if not, please tell us why
  here.

• [x] 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 adds a stack_load and stack_store encoding for 32bit and 64bit integers. This saves one lea instruction for each stack_load and stack_store.

• [x] 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

<strike>This currently doesn't encode stack_load and stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00
```</strike>
~~~

GitHub (Jan 25 2020 at 14:11):

bjorn3 edited PR #1304 from encode_stack_load_store to master:

• [x] This has been discussed in issue #597, or if not, please tell us why
  here.

• [x] 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 adds a stack_load and stack_store encoding for 32bit and 64bit integers. This saves one lea instruction for each stack_load and stack_store.

• [x] 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

<strike>
This currently doesn't encode stack_load and stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00

</strike>

GitHub (Jan 25 2020 at 14:11):

bjorn3 requested bnjbvr for a review on PR #1304.

GitHub (Jan 25 2020 at 14:11):

bjorn3 edited PR #1304 from encode_stack_load_store to master:

• [x] This has been discussed in issue #597, or if not, please tell us why
  here.

• [x] 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 adds a stack_load and stack_store encoding for 32bit and 64bit integers. This saves one lea instruction for each stack_load and stack_store.

• [x] This PR contains test cases, if meaningful.

• [x] 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

<strike>
This currently doesn't encode stack_load and stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00

</strike>

GitHub (Feb 13 2020 at 10:40):

bnjbvr submitted PR Review.

GitHub (Feb 13 2020 at 10:40):

bnjbvr created PR Review Comment:

Why is this needed here and not in the load case?

GitHub (Feb 13 2020 at 10:40):

bnjbvr submitted PR Review.

GitHub (Feb 13 2020 at 10:40):

bnjbvr created PR Review Comment:

Is it possible to use the 8-bytes displacement on x86 32 bits, though? It seems like a possible regression, and one that's hard to test, since we only test that the generated machine code matches our encodings.

(One possible way to resolve this would be to just remove this line, in light of the plan to have a new isel/codegen mechanism, since x86 32 bits is badly supported anyways.)

GitHub (Feb 13 2020 at 11:48):

bjorn3 submitted PR Review.

GitHub (Feb 13 2020 at 11:48):

bjorn3 created PR Review Comment:

I followed the example of the spill and fill encodings. Stack loads should always have a stack store before them to prevent reading undefined bytes, so any stack overflow would have already been catched by the stack store. Unlike the fill instruction, a stack load of undefined bytes is possible, so maybe I should add a StackOverflow trap marker to load anyway.

GitHub (Feb 13 2020 at 11:49):

bjorn3 submitted PR Review.

GitHub (Feb 13 2020 at 11:49):

bjorn3 created PR Review Comment:

I can add the 8bit displacement back.

GitHub (Feb 13 2020 at 16:04):

iximeow submitted PR Review.

GitHub (Feb 13 2020 at 16:04):

iximeow created PR Review Comment:

I'm not sure if @bnjbvr meant 8-byte or 8-bit displacement, but to be clear, x86 does not support 8-byte displacements.

I'm suspect of the spaddr4 encoding for x86 32-bit that was present since while it used modrm_sib_disp8(out_reg0, sink); to prepare for an 8-bit displacement, it proceeded to write four bytes of displacement out with sink.put4(sp.offset.checked_add(imm).unwrap() as u32);

I think adding keeping an 8-bit displacement form would be good though, as it would be useful for functions with smaller stack regions.

GitHub (Feb 13 2020 at 17:10):

bjorn3 submitted PR Review.

GitHub (Feb 13 2020 at 17:10):

bjorn3 created PR Review Comment:

Yeah, that put4 was the reason I removed it.

GitHub (Feb 29 2020 at 14:25):

bjorn3 updated PR #1304 from encode_stack_load_store to master:

• [x] This has been discussed in issue #597, or if not, please tell us why
  here.

• [x] 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 adds a stack_load and stack_store encoding for 32bit and 64bit integers. This saves one lea instruction for each stack_load and stack_store.

• [x] This PR contains test cases, if meaningful.

• [x] 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 and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.

<strike>
This currently doesn't encode stack_load and stack_store correctly.

[RexOp1spst_id#8089]                stack_store v0, ss1
[...]
@0000 [RexOp1spaddr_ld_id#808b,%rax] v20 = stack_load.i64 ss1

results in

  4a3df4:       48 89 bc 24 40 00 00    mov    %rdi,0x40(%rsp)
  4a3dfb:       00
[...]
  4a3e04:       48 8b 84 24 40 00 00    mov    0x40(%rsp),%rax
  4a3e0b:       00

</strike>

GitHub (Feb 29 2020 at 14:59):

bjorn3 closed without merge PR #1304.

Last updated: Dec 23 2024 at 14:03 UTC