Wasmtime GitHub notifications bot (Jul 11 2020 at 00:24): Wasmtime GitHub notifications bot (Jul 11 2020 at 00:24):jlb6740 opened PR #2011 from vcode-add-fcmp to main:
<!--
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 (Jul 11 2020 at 00:27):jlb6740 requested bnjbvr for a review on PR #2011.
Wasmtime GitHub notifications bot (Jul 11 2020 at 01:29):jlb6740 updated PR #2011 from vcode-add-fcmp to main:
<!--
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 (Jul 15 2020 at 01:44):jlb6740 updated PR #2011 from vcode-add-fcmp to main:
<!--
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 (Jul 15 2020 at 14:36):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jul 15 2020 at 14:36):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jul 15 2020 at 14:36):bnjbvr created PR Review Comment:
nit: can you group all the
unimplemented!cases with|, please?
Also a small typo in the error message:guarantee
Wasmtime GitHub notifications bot (Jul 15 2020 at 14:36):bnjbvr created PR Review Comment:
Can you add emission tests for ucomiss, please?
Wasmtime GitHub notifications bot (Jul 15 2020 at 14:36):bnjbvr created PR Review Comment:
Not sure it's already included in the commits you're basing your work from, but: latest commit adding Bitcast contains a new helper to check that the
RegMemoperand is aV128reg, if it's a reg:assert_reg_type. If you have it in your history, can you use it here please? Otherwise, can you add a TODO here to use it?
Wasmtime GitHub notifications bot (Jul 15 2020 at 14:36):bnjbvr created PR Review Comment:
nit: missing word after
benefit by? (or at least an empty space to remove)
Wasmtime GitHub notifications bot (Jul 15 2020 at 14:36):bnjbvr created PR Review Comment:
Reversing operands sounds great here!
Wasmtime GitHub notifications bot (Jul 15 2020 at 14:36):bnjbvr created PR Review Comment:
typo:
checks two or fewer
Wasmtime GitHub notifications bot (Jul 15 2020 at 14:36):bnjbvr created PR Review Comment:
nit: can you either check if the input type is f32 or f64 and select the right among Ucomiss/Ucomisd here, or add an
unimplementedstatement if the input type is f64, please?
Wasmtime GitHub notifications bot (Jul 15 2020 at 14:36):bnjbvr created PR Review Comment:
typo:
namely
Wasmtime GitHub notifications bot (Jul 15 2020 at 14:36):bnjbvr created PR Review Comment:
nit: unfinished comment here.
It seems the old backend would generate two comparisons + setcc for equality and non-equality, and
andthe results; are these the two only cases where we can't just use a singlesetccinstructions?
Alternatively, it seems that Spidermonkey will do an inline test for the parity flag and jump inline to set the destination result to the final value in this case. Doing likewise would require this instruction to become a synthetic sequence, so the branch and jump aren't separated by a spill/reload.
Wasmtime GitHub notifications bot (Jul 15 2020 at 23:36):jlb6740 submitted PR Review.
Wasmtime GitHub notifications bot (Jul 15 2020 at 23:36):jlb6740 created PR Review Comment:
I've cleaned all this up.
Wasmtime GitHub notifications bot (Jul 15 2020 at 23:46):jlb6740 submitted PR Review.
Wasmtime GitHub notifications bot (Jul 15 2020 at 23:46):jlb6740 created PR Review Comment:
nit: unfinished comment here.
It seems the old backend would generate two comparisons + setcc for equality and non-equality, and
andthe results; are these the two only cases where we can't just use a singlesetccinstructions?I saw that about non-equality and was confused by it. Non-equality should require a check that the ZF is set to 0. If that is the case then the parity bit cannot be set. From what I see the only case supported in cranelift that needs a separate check is the equality condition. The llvm code that I was looking at does have a case for unordered or non-equality that I think also requires a separate check.
Alternatively, it seems that Spidermonkey will do an inline test for the parity flag and jump inline to set the destination result to the final value in this case. Doing likewise would require this instruction to become a synthetic sequence, so the branch and jump aren't separated by a spill/reload.
Not quote sure I follow here but would like to see this code to understand. I had considered that a cmov would potentially be useful here but it doesn't seem jmps or cmovs will be necessary.
Wasmtime GitHub notifications bot (Jul 16 2020 at 00:13):jlb6740 edited PR Review Comment.
Wasmtime GitHub notifications bot (Jul 16 2020 at 00:25):jlb6740 updated PR #2011 from vcode-add-fcmp to main:
<!--
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 (Jul 16 2020 at 00:26):jlb6740 has marked PR #2011 as ready for review.
Wasmtime GitHub notifications bot (Jul 16 2020 at 06:02):jlb6740 updated PR #2011 from vcode-add-fcmp to main:
<!--
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 (Jul 16 2020 at 06:04):jlb6740 updated PR #2011 from vcode-add-fcmp to main:
<!--
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 (Jul 16 2020 at 06:33):jlb6740 updated PR #2011 from vcode-add-fcmp to main:
<!--
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 (Jul 16 2020 at 06:37):jlb6740 submitted PR Review.
Wasmtime GitHub notifications bot (Jul 16 2020 at 06:37):jlb6740 created PR Review Comment:
I've added ucomisd support to this patch.
Wasmtime GitHub notifications bot (Jul 16 2020 at 06:41):jlb6740 submitted PR Review.
Wasmtime GitHub notifications bot (Jul 16 2020 at 06:41):jlb6740 created PR Review Comment:
I can not find this symbol in my tree. This patch needs to be rebased but it is likely really a child patch or two that needs rebasing. I have not rebased this because I am not sure how best to proceed.
Wasmtime GitHub notifications bot (Jul 16 2020 at 06:50):jlb6740 updated PR #2011 from vcode-add-fcmp to main:
<!--
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 (Jul 16 2020 at 06:50):jlb6740 edited PR Review Comment.
Wasmtime GitHub notifications bot (Jul 16 2020 at 07:02):jlb6740 updated PR #2011 from vcode-add-fcmp to main:
<!--
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 (Jul 16 2020 at 07:05):jlb6740 requested bnjbvr for a review on PR #2011.
Wasmtime GitHub notifications bot (Jul 16 2020 at 15:12):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jul 16 2020 at 15:12):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jul 16 2020 at 15:12):bnjbvr created PR Review Comment:
typo: exclude
Wasmtime GitHub notifications bot (Jul 16 2020 at 15:12):bnjbvr created PR Review Comment:
typo: exclude
Wasmtime GitHub notifications bot (Jul 16 2020 at 15:12):bnjbvr created PR Review Comment:
nit: rebase error, i assume
Wasmtime GitHub notifications bot (Jul 16 2020 at 15:12):bnjbvr created PR Review Comment:
dstis redefined exactly the same way here, so we can get rid of it.Also, it's not ideal to redefine lhs and rhs here (those
input_to_regfunctions may have side-effects). Instead, can you remove the definition of lhs and rhs that live above the match, and redefine them in each arm, please? (So nothing to do for lhs and rhs here, only redefine them properly in the two other arms.)\(Unless you find an even better way, of course!)
Wasmtime GitHub notifications bot (Jul 16 2020 at 15:12):bnjbvr created PR Review Comment:
typo:
ordreed
Wasmtime GitHub notifications bot (Jul 17 2020 at 13:15):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jul 17 2020 at 18:17):jlb6740 submitted PR Review.
Wasmtime GitHub notifications bot (Jul 17 2020 at 18:17):jlb6740 created PR Review Comment:
:+1: I guess. I've added it back.
Wasmtime GitHub notifications bot (Jul 17 2020 at 18:27):jlb6740 updated PR #2011 from vcode-add-fcmp to main:
<!--
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 (Jul 17 2020 at 20:46):jlb6740 merged PR #2011.
Last updated: Dec 23 2024 at 13:07 UTC