Stream: git-cranelift

Topic: cranelift / PR #1325 When splitting a const, insert prior...

GitHub (Jan 08 2020 at 22:55):

sstangl requested bnjbvr for a review on PR #1325.

GitHub (Jan 08 2020 at 22:55):

sstangl opened PR #1325 from bb-isplit-1159 to master:

Closes #1159

Given code like the following, on x86_64, which does not have i128 registers:

ebb0(v0: i64):
    v1 = iconst.i128 0
    v2 = icmp_imm eq v0, 1
    brnz v2, ebb1
    jump ebb2(v1)

It would be split to:

ebb0(v0: i64):
    v1 = iconst.i128 0
    v2 = icmp_imm eq v0, 1
    brnz v2, ebb1
    v3, v4 = isplit.i128 v1
    jump ebb2(v3, v4)

But that fails basic-block invariants. This patch changes that to:

ebb0(v0: i64):
    v1 = iconst.i128 0
    v2 = icmp_imm eq v0, 1
    v3, v4 = isplit.i128 v1
    brnz v2, ebb1
    jump ebb2(v3, v4)

• [x] This has been discussed in issue #1159 .

I decided to move the isplit before the terminal branch group to keep the invariants simple: in that issue it was suggested that we solve this by allowing isplit instructions within the branch group, but in prior patches it was common need to have to inspect multiple branches in the same terminal group. If we allowed isplit there, many places throughout the code would have to learn how to ignore those instructions. Moving the isplit prior to all branches is a lower cognitive burden.

• [x] A short description of what this does, why it is needed.
• [x] This PR contains test cases, if meaningful.

The testcase fails on master (with basic-blocks disabled) because iconst.i128 still cannot be legalized. This patch causes the same behavior with/without basic-blocks.

• [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.

GitHub (Jan 09 2020 at 20:48):

sstangl updated PR #1325 from bb-isplit-1159 to master:

Closes #1159

Given code like the following, on x86_64, which does not have i128 registers:

ebb0(v0: i64):
    v1 = iconst.i128 0
    v2 = icmp_imm eq v0, 1
    brnz v2, ebb1
    jump ebb2(v1)

It would be split to:

ebb0(v0: i64):
    v1 = iconst.i128 0
    v2 = icmp_imm eq v0, 1
    brnz v2, ebb1
    v3, v4 = isplit.i128 v1
    jump ebb2(v3, v4)

But that fails basic-block invariants. This patch changes that to:

ebb0(v0: i64):
    v1 = iconst.i128 0
    v2 = icmp_imm eq v0, 1
    v3, v4 = isplit.i128 v1
    brnz v2, ebb1
    jump ebb2(v3, v4)

• [x] This has been discussed in issue #1159 .

I decided to move the isplit before the terminal branch group to keep the invariants simple: in that issue it was suggested that we solve this by allowing isplit instructions within the branch group, but in prior patches it was common need to have to inspect multiple branches in the same terminal group. If we allowed isplit there, many places throughout the code would have to learn how to ignore those instructions. Moving the isplit prior to all branches is a lower cognitive burden.

• [x] A short description of what this does, why it is needed.
• [x] This PR contains test cases, if meaningful.

The testcase fails on master (with basic-blocks disabled) because iconst.i128 still cannot be legalized. This patch causes the same behavior with/without basic-blocks.

• [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.

GitHub (Jan 09 2020 at 20:48):

sstangl edited PR #1325 from bb-isplit-1159 to master:

Closes #1159

Given code like the following, on x86_64, which does not have i128 registers:

ebb0(v0: i64):
    v1 = iconst.i128 0
    v2 = icmp_imm eq v0, 1
    brnz v2, ebb1
    jump ebb2(v1)

It would be split to:

ebb0(v0: i64):
    v1 = iconst.i128 0
    v2 = icmp_imm eq v0, 1
    brnz v2, ebb1
    v3, v4 = isplit.i128 v1
    jump ebb2(v3, v4)

But that fails basic-block invariants. This patch changes that to:

ebb0(v0: i64):
    v1 = iconst.i128 0
    v2 = icmp_imm eq v0, 1
    v3, v4 = isplit.i128 v1
    brnz v2, ebb1
    jump ebb2(v3, v4)

• [x] This has been discussed in issue #1159 .

I decided to move the isplit before the terminal branch group to keep the invariants simple: in that issue it was suggested that we solve this by allowing isplit instructions within the branch group, but in prior patches it was common need to have to inspect multiple branches in the same terminal group. If we allowed isplit there, many places throughout the code would have to learn how to ignore those instructions. Moving the isplit prior to all branches is a lower cognitive burden.

• [x] A short description of what this does, why it is needed.
• [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.

GitHub (Jan 10 2020 at 09:11):

bjorn3 submitted PR Review.

GitHub (Jan 10 2020 at 10:23):

bnjbvr submitted PR Review.

GitHub (Jan 10 2020 at 10:23):

bnjbvr created PR Review Comment:

Can you add test annotations to check that the compiled form is what we expect, here, please?

GitHub (Jan 10 2020 at 10:23):

bnjbvr created PR Review Comment:

Is this split into its own variable to avoid borrowing pos.func twice? If not, since it's used only once can we inline it at its use?

GitHub (Jan 10 2020 at 10:23):

bnjbvr submitted PR Review.

GitHub (Jan 22 2020 at 16:14):

bnjbvr merged PR #1325.

GitHub (Jan 22 2020 at 17:17):

bnjbvr submitted PR Review.

GitHub (Jan 22 2020 at 17:17):

bnjbvr created PR Review Comment:

Actually we can't check for the split instruction being present in the output, because we can reuse a pair created by concat, it seems.

Last updated: Dec 23 2024 at 13:07 UTC