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 ani32
constant was used in bit-twiddling logic.Add support for
iadd_ifcout
in 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_label
functionality rather than touching unimplemented APIs.
This is not a bugfix per-se, but removes confusing panic messages from
cargo test
output 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.
-->
cfallin requested bnjbvr for a review on PR #1825.
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 ani32
constant was used in bit-twiddling logic.Add support for
iadd_ifcout
in 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_label
functionality rather than touching unimplemented APIs.
This is not a bugfix per-se, but removes confusing panic messages from
cargo test
output 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.
-->
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 ani32
constant was used in bit-twiddling logic.Add support for
iadd_ifcout
in 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_label
functionality rather than touching unimplemented APIs.
This is not a bugfix per-se, but removes confusing panic messages from
cargo test
output 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.
-->
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 ani32
constant was used in bit-twiddling logic.Add support for
iadd_ifcout
in 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_label
functionality rather than touching unimplemented APIs.
This is not a bugfix per-se, but removes confusing panic messages from
cargo test
output 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.
-->
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
nit: corresponds
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
nit: corresponds
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.
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 ani32
constant was used in bit-twiddling logic.Add support for
iadd_ifcout
in 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_label
functionality rather than touching unimplemented APIs.
This is not a bugfix per-se, but removes confusing panic messages from
cargo test
output 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.
-->
cfallin submitted PR Review.
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.
cfallin merged PR #1825.
Last updated: Dec 23 2024 at 12:05 UTC