Stream: git-wasmtime

Topic: wasmtime / PR #5630 cranelift: Remove brz and brnz

Wasmtime GitHub notifications bot (Jan 25 2023 at 01:07):

elliottt opened PR #5630 from trevor/remove-brz-brnz to main:

• Remove brz and brnz
• Start removing
• Update filetests
• Update more tests
• Update function generation for fuzzing
• Fix frontend tests
• Fix cranelift-wasm
• Format

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

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

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jan 25 2023 at 04:52):

elliottt updated PR #5630 from trevor/remove-brz-brnz to main.

Wasmtime GitHub notifications bot (Jan 25 2023 at 04:53):

elliottt edited PR #5630 from trevor/remove-brz-brnz to main:

Remove the brz and brnz instructions, as their behavior is now redundant with brif.
<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

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

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jan 25 2023 at 17:05):

elliottt updated PR #5630 from trevor/remove-brz-brnz to main.

Wasmtime GitHub notifications bot (Jan 25 2023 at 17:06):

elliottt updated PR #5630 from trevor/remove-brz-brnz to main.

Wasmtime GitHub notifications bot (Jan 25 2023 at 17:26):

elliottt updated PR #5630 from trevor/remove-brz-brnz to main.

Wasmtime GitHub notifications bot (Jan 25 2023 at 18:28):

elliottt updated PR #5630 from trevor/remove-brz-brnz to main.

Wasmtime GitHub notifications bot (Jan 25 2023 at 19:11):

elliottt updated PR #5630 from trevor/remove-brz-brnz to main.

Wasmtime GitHub notifications bot (Jan 25 2023 at 19:18):

elliottt has marked PR #5630 as ready for review.

Wasmtime GitHub notifications bot (Jan 25 2023 at 19:19):

elliottt edited PR #5630 from trevor/remove-brz-brnz to main:

Remove the brz and brnz instructions, as their behavior is now redundant with brif. I've also updated docs and removed all references in comments to brz and brnz.
<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

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

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jan 25 2023 at 21:27):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2023 at 21:27):

elliottt created PR review comment:

The TODO on line 523 is referring to this unnecessary indirection introduced by the switch compilation. I'm not sure if there's a good way to rework build_search_branches to avoid this, but perhaps it's not worth thinking too hard about as VCode will skip the intermeidate block.

Wasmtime GitHub notifications bot (Jan 25 2023 at 21:28):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2023 at 21:28):

elliottt created PR review comment:

Similarly, this unnecessary intermediate block is introduced as an effect of the changes to build_search_branches, and is the subject of the TODO on line 451.

Wasmtime GitHub notifications bot (Jan 25 2023 at 21:31):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2023 at 21:31):

elliottt created PR review comment:

This PR introduced a lot of noise for cases where a brz was replaced with a brif with the branch order reversed. This change is equivalent in behavior to the previous output, but ends up changing the label names.

Wasmtime GitHub notifications bot (Jan 25 2023 at 21:44):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2023 at 21:44):

elliottt created PR review comment:

This ballooned a bit because it's now testing ult, rather than the negation of ult which is just ge. We could get back to the old implementation by swapping the branches and inverting the condition code.

Wasmtime GitHub notifications bot (Jan 25 2023 at 22:12):

elliottt requested cfallin for a review on PR #5630.

Wasmtime GitHub notifications bot (Jan 25 2023 at 22:12):

elliottt requested fitzgen for a review on PR #5630.

Wasmtime GitHub notifications bot (Jan 26 2023 at 01:33):

elliottt updated PR #5630 from trevor/remove-brz-brnz to main.

Wasmtime GitHub notifications bot (Jan 26 2023 at 01:33):

elliottt updated PR #5630 from trevor/remove-brz-brnz to main.

Wasmtime GitHub notifications bot (Jan 26 2023 at 22:20):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 26 2023 at 22:20):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 26 2023 at 22:20):

fitzgen created PR review comment:

:tada:

Wasmtime GitHub notifications bot (Jan 26 2023 at 22:20):

fitzgen created PR review comment:

Is this something we should have a lowering rule for? Fine to file as a follow up issue, IMO.

Wasmtime GitHub notifications bot (Jan 26 2023 at 22:20):

fitzgen created PR review comment:

Can this reuse the existing temporary SmallVec just above above?

Wasmtime GitHub notifications bot (Jan 26 2023 at 23:06):

elliottt updated PR #5630 from trevor/remove-brz-brnz to main.

Wasmtime GitHub notifications bot (Jan 26 2023 at 23:08):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 26 2023 at 23:08):

elliottt created PR review comment:

I fixed this separately in #5636. I realized we could invert the condition code and branches to avoid the cost of the unordered comparison in the riscv64 backend :+1:

Wasmtime GitHub notifications bot (Jan 26 2023 at 23:10):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 26 2023 at 23:10):

elliottt created PR review comment:

Unfortunately no, as canonicalise_v128_values returns the smallvec as a slice, and we need the two slices at the same time.

Wasmtime GitHub notifications bot (Jan 27 2023 at 01:47):

elliottt updated PR #5630 from trevor/remove-brz-brnz to main.

Wasmtime GitHub notifications bot (Jan 28 2023 at 00:01):

elliottt updated PR #5630 from trevor/remove-brz-brnz to main.

Wasmtime GitHub notifications bot (Jan 28 2023 at 00:08):

elliottt has marked PR #5630 as ready for review.

Wasmtime GitHub notifications bot (Jan 28 2023 at 01:32):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jan 28 2023 at 01:32):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jan 28 2023 at 01:32):

jameysharp created PR review comment:

I don't know exactly what use_srcloc does but does using .replace(inst) above effectively do the same thing as this?

Wasmtime GitHub notifications bot (Jan 28 2023 at 01:32):

jameysharp created PR review comment:

Just to check: is this fixing a bug introduced in one of the previous PRs in this series?

Wasmtime GitHub notifications bot (Jan 28 2023 at 01:34):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jan 28 2023 at 01:34):

jameysharp created PR review comment:

I believe this test is now identical to i128_brif_false above, right?

Wasmtime GitHub notifications bot (Jan 28 2023 at 01:36):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jan 28 2023 at 01:36):

jameysharp created PR review comment:

And same for all the other *_brif_false/*_brif_true tests below.

Wasmtime GitHub notifications bot (Jan 30 2023 at 17:40):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 30 2023 at 17:40):

elliottt created PR review comment:

I think that that the effects of use_srcloc are lost in my new version, as any instructions inserted using poswill not inherit that location. Thanks for catching this!

Wasmtime GitHub notifications bot (Jan 30 2023 at 17:58):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 30 2023 at 17:58):

elliottt created PR review comment:

Yes. The BlockCall pr was unconditionally removing parameters from all branches, so any use of bugpoint::run with brif would potentially produce brif calls with incorrect block arguments.

Wasmtime GitHub notifications bot (Jan 30 2023 at 19:39):

elliottt updated PR #5630 from trevor/remove-brz-brnz to main.

Wasmtime GitHub notifications bot (Jan 30 2023 at 19:44):

elliottt has enabled auto merge for PR #5630.

Wasmtime GitHub notifications bot (Jan 30 2023 at 20:34):

elliottt merged PR #5630.

Last updated: Dec 23 2024 at 12:05 UTC