Wasmtime GitHub notifications bot (Jun 05 2020 at 02:22): Wasmtime GitHub notifications bot (Jun 05 2020 at 02:22):cfallin opened PR #1825 from spidermonkey-fixes to master:
Properly mask constant values down to appropriate width when
generating a constant value directly in aarch64 backend. This was a
miscompilation introduced in the new-isel refactor. In combination
with failure to respect NarrowValueMode, this resulted in a very
subtle bug when ani32constant was used in bit-twiddling logic.Add support for
iadd_ifcoutin aarch64 backend as used in explicit
heap-check mode. With this change, we no longer fail heap-related
tests with the huge-heap-region mode disabled.Remove a panic that was occurring in some tests that are currently
ignored on aarch64, by simply returning empty/default information in
value_labelfunctionality rather than touching unimplemented APIs.
This is not a bugfix per-se, but removes confusing panic messages from
cargo testoutput that might otherwise mislead.<!--
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 (Jun 05 2020 at 02:22):cfallin requested bnjbvr for a review on PR #1825.
Wasmtime GitHub notifications bot (Jun 05 2020 at 22:34):cfallin updated PR #1825 from spidermonkey-fixes to master:
Properly mask constant values down to appropriate width when
generating a constant value directly in aarch64 backend. This was a
miscompilation introduced in the new-isel refactor. In combination
with failure to respect NarrowValueMode, this resulted in a very
subtle bug when ani32constant was used in bit-twiddling logic.Add support for
iadd_ifcoutin aarch64 backend as used in explicit
heap-check mode. With this change, we no longer fail heap-related
tests with the huge-heap-region mode disabled.Remove a panic that was occurring in some tests that are currently
ignored on aarch64, by simply returning empty/default information in
value_labelfunctionality rather than touching unimplemented APIs.
This is not a bugfix per-se, but removes confusing panic messages from
cargo testoutput that might otherwise mislead.<!--
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 (Jun 05 2020 at 23:12):cfallin updated PR #1825 from spidermonkey-fixes to master:
Properly mask constant values down to appropriate width when
generating a constant value directly in aarch64 backend. This was a
miscompilation introduced in the new-isel refactor. In combination
with failure to respect NarrowValueMode, this resulted in a very
subtle bug when ani32constant was used in bit-twiddling logic.Add support for
iadd_ifcoutin aarch64 backend as used in explicit
heap-check mode. With this change, we no longer fail heap-related
tests with the huge-heap-region mode disabled.Remove a panic that was occurring in some tests that are currently
ignored on aarch64, by simply returning empty/default information in
value_labelfunctionality rather than touching unimplemented APIs.
This is not a bugfix per-se, but removes confusing panic messages from
cargo testoutput that might otherwise mislead.<!--
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 (Jun 06 2020 at 00:30):cfallin updated PR #1825 from spidermonkey-fixes to master:
Properly mask constant values down to appropriate width when
generating a constant value directly in aarch64 backend. This was a
miscompilation introduced in the new-isel refactor. In combination
with failure to respect NarrowValueMode, this resulted in a very
subtle bug when ani32constant was used in bit-twiddling logic.Add support for
iadd_ifcoutin aarch64 backend as used in explicit
heap-check mode. With this change, we no longer fail heap-related
tests with the huge-heap-region mode disabled.Remove a panic that was occurring in some tests that are currently
ignored on aarch64, by simply returning empty/default information in
value_labelfunctionality rather than touching unimplemented APIs.
This is not a bugfix per-se, but removes confusing panic messages from
cargo testoutput that might otherwise mislead.<!--
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 (Jun 08 2020 at 16:47):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jun 08 2020 at 16:47):bnjbvr created PR Review Comment:
nit: corresponds
Wasmtime GitHub notifications bot (Jun 08 2020 at 16:47):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jun 08 2020 at 16:47):bnjbvr created PR Review Comment:
nit: corresponds
Wasmtime GitHub notifications bot (Jun 08 2020 at 16:47):bnjbvr created PR Review Comment:
It was UnsignedLessThan in the old backend's code; was this a preexisting bug, or is this a bad copy-pasto? I'm not sure it's quite easy to test right now, because there's no support for x86-64 in Spidermonkey, and I'm convinced it's testable with Cranelift's old x86 backend in Spidermonkey.
Wasmtime GitHub notifications bot (Jun 08 2020 at 20:02):cfallin updated PR #1825 from spidermonkey-fixes to master:
Properly mask constant values down to appropriate width when
generating a constant value directly in aarch64 backend. This was a
miscompilation introduced in the new-isel refactor. In combination
with failure to respect NarrowValueMode, this resulted in a very
subtle bug when ani32constant was used in bit-twiddling logic.Add support for
iadd_ifcoutin aarch64 backend as used in explicit
heap-check mode. With this change, we no longer fail heap-related
tests with the huge-heap-region mode disabled.Remove a panic that was occurring in some tests that are currently
ignored on aarch64, by simply returning empty/default information in
value_labelfunctionality rather than touching unimplemented APIs.
This is not a bugfix per-se, but removes confusing panic messages from
cargo testoutput that might otherwise mislead.<!--
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 (Jun 08 2020 at 20:03):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jun 08 2020 at 20:03):cfallin created PR Review Comment:
It's unclear to me why it was the way it was; but with the way that things are lowered now, it should be
>=, as far as I can tell. The documentation for the underlying trait method this fulfills (onTargetIsa) says that it is the condition that should be triggered when an add overflows; and that's definitely carry-flag-set on x86 (and aarch64). In any case we will want to carefully check this when we build out the new x86-64 backend.
Wasmtime GitHub notifications bot (Jun 08 2020 at 20:54):cfallin merged PR #1825.
Last updated: Nov 22 2024 at 16:03 UTC