Stream: git-wasmtime

Topic: wasmtime / PR #7593 PCC: Fix several aarch64 check_consta...

Wasmtime GitHub notifications bot (Nov 28 2023 at 12:59):

feilongjiang requested abrown for a review on PR #7593.

Wasmtime GitHub notifications bot (Nov 28 2023 at 12:59):

feilongjiang requested wasmtime-compiler-reviewers for a review on PR #7593.

Wasmtime GitHub notifications bot (Nov 28 2023 at 12:59):

feilongjiang opened PR #7593 from feilongjiang:fix_arm64_pcc_check_constant to bytecodealliance:main:

PCC: Fix several aarch64 check_constant failures
This patch fixes several aarch64 check_constant failures:

1. check_subsume for AluRRImmLogic failed due to the mismatch of fact width

   isle rule for orr_imm always generates fact with bit_width 64 regardless of
   immediate type. So when Type is I32, check_subsume will be failed.
   This could be reproducedwith the simple iconst.i32 wasm module [1].

2. MovN generates incorrect fact range value when first_is_inverted is true

   running_value should be calculated as !(((!imm16) & 0xffff) << shift) or
   !(u64::from(imm.bits) << shift)
   This could be reproduced with the simple iconst.i64 wasm module [2].

Added two test cases in cranelift/filetests/filetests/pcc/succeed/const.clif.

Additional fix for get_fact_or_default:

trace! treats reg as VirtualReg and it will panic when "reg" is RealReg and
trace-log feature is enabled.

[1]:

;; test_logic.wat
(module
  (func (export "test_logic") (result i32) (i32.const 0x100010))
)

[2]:

;; test_const.wat
(module
  (func (export "test_constant") (result i64) (i64.const 0x0009ffffffff))
)

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Nov 28 2023 at 12:59):

feilongjiang edited PR #7593:

Hi team, please consider
This patch fixes several aarch64 check_constant failures:

1. check_subsume for AluRRImmLogic failed due to the mismatch of fact width

   isle rule for orr_imm always generates fact with bit_width 64 regardless of
   immediate type. So when Type is I32, check_subsume will be failed.
   This could be reproduced with the simple wasm module [1].

2. MovN generates incorrect fact range value when first_is_inverted is true

   running_value should be calculated as !(((!imm16) & 0xffff) << shift) or
   !(u64::from(imm.bits) << shift)
   This could be reproduced with the simple wasm module [2].

Added two test cases in cranelift/filetests/filetests/pcc/succeed/const.clif.

Additional fix for get_fact_or_default:

trace! treats reg as VirtualReg and it will panic when "reg" is RealReg and
trace-log feature is enabled.

[1]:

;; test_logic.wat
(module
  (func (export "test_logic") (result i32) (i32.const 0x100010))
)

[2]:

;; test_const.wat
(module
  (func (export "test_constant") (result i64) (i64.const 0x0009ffffffff))
)

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Nov 28 2023 at 13:00):

feilongjiang edited PR #7593:

Hi team, please consider:
This patch fixes several aarch64 check_constant failures:

1. check_subsume for AluRRImmLogic failed due to the mismatch of fact width

   isle rule for orr_imm always generates fact with bit_width 64 regardless of
   immediate type. So when Type is I32, check_subsume will be failed.
   This could be reproduced with the simple wasm module [1].

2. MovN generates incorrect fact range value when first_is_inverted is true

   running_value should be calculated as !(((!imm16) & 0xffff) << shift) or
   !(u64::from(imm.bits) << shift)
   This could be reproduced with the simple wasm module [2].

Added two test cases in cranelift/filetests/filetests/pcc/succeed/const.clif.

Additional fix for get_fact_or_default:

trace! treats reg as VirtualReg and it will panic when "reg" is RealReg and
trace-log feature is enabled.

[1]:

;; test_logic.wat
(module
  (func (export "test_logic") (result i32) (i32.const 0x100010))
)

[2]:

;; test_const.wat
(module
  (func (export "test_constant") (result i64) (i64.const 0x0009ffffffff))
)

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Nov 28 2023 at 13:01):

feilongjiang edited PR #7593:

Hi team, please consider:
This patch fixes several aarch64 check_constant failures:

1. check_subsume for AluRRImmLogic failed due to the mismatch of fact width

   isle rule for orr_imm always generates fact with bit_width 64 regardless of
   immediate type. So when Type is I32, check_subsume will be failed.
   This could be reproduced with the simple wasm module [1].

2. MovN generates incorrect fact range value when first_is_inverted is true

   running_value should be calculated as !(((!imm16) & 0xffff) << shift) or
   !(u64::from(imm.bits) << shift)
   This could be reproduced with the simple wasm module [2].

Added two test cases in cranelift/filetests/filetests/pcc/succeed/const.clif.

Additional fix for get_fact_or_default:

trace! treats reg as VirtualReg and it will panic when "reg" is RealReg and
trace-log feature is enabled.

[1]:

;; test_logic.wat
(module
  (func (export "test_logic") (result i32) (i32.const 0x100010))
)

[2]:

;; test_const.wat
(module
  (func (export "test_constant") (result i64) (i64.const 0x0009ffffffff))
)

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Nov 29 2023 at 09:14):

feilongjiang edited PR #7593:

Hi team, please consider:
This patch fixes several aarch64 check_constant failures:

1. check_subsume for AluRRImmLogic failed due to the mismatch of fact width

   isle rule for orr_imm always generates fact with bit_width 64 regardless of
   immediate type. So when Type is I32, check_subsume will be failed.
   This could be reproduced with the simple wasm module [1].

2. MovN generates incorrect fact range value when first_is_inverted is true

   running_value should be calculated as !(((!imm16) & 0xffff) << shift) or
   !(u64::from(imm.bits) << shift)
   This could be reproduced with the simple wasm module [2].

Added two test cases in cranelift/filetests/filetests/pcc/succeed/const.clif.

Additional fix for get_fact_or_default:

trace! treats reg as VirtualReg and it will panic when reg is RealReg and
trace-log feature is enabled.

[1]:

;; test_logic.wat
(module
  (func (export "test_logic") (result i32) (i32.const 0x100010))
)

[2]:

;; test_const.wat
(module
  (func (export "test_constant") (result i64) (i64.const 0x0009ffffffff))
)

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Nov 29 2023 at 16:13):

alexcrichton commented on PR #7593:

Thanks for the PR!

The two folks who are best to review this PR, @cfallin and @fitzgen, are both out on leave right now. They'll get to this PR when they get back though (@fitzgen's coming back mid-December). Thanks for your patience

Wasmtime GitHub notifications bot (Nov 29 2023 at 21:44):

alexcrichton requested fitzgen for a review on PR #7593.

Wasmtime GitHub notifications bot (Dec 11 2023 at 19:42):

fitzgen submitted PR review:

LGTM with one suggestion to try before we merge this.

Wasmtime GitHub notifications bot (Dec 11 2023 at 19:42):

fitzgen submitted PR review:

LGTM with one suggestion to try before we merge this.

Wasmtime GitHub notifications bot (Dec 11 2023 at 19:42):

fitzgen created PR review comment:

I think this can be simplified to

        "get_fact_or_default: reg {reg:?} -> {:?}"

Wasmtime GitHub notifications bot (Dec 11 2023 at 19:42):

fitzgen submitted PR review:

LGTM with one suggestion to try before we merge this. Thanks!

Wasmtime GitHub notifications bot (Dec 11 2023 at 19:42):

fitzgen commented on PR #7593:

And thanks for your patience!

Wasmtime GitHub notifications bot (Dec 12 2023 at 09:40):

feilongjiang updated PR #7593.

Wasmtime GitHub notifications bot (Dec 12 2023 at 09:41):

feilongjiang commented on PR #7593:

@fitzgen Thanks for the review and suggestions!

Wasmtime GitHub notifications bot (Dec 12 2023 at 09:46):

feilongjiang updated PR #7593.

Wasmtime GitHub notifications bot (Dec 12 2023 at 13:03):

feilongjiang requested fitzgen for a review on PR #7593.

Wasmtime GitHub notifications bot (Dec 12 2023 at 19:37):

fitzgen submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Dec 12 2023 at 20:13):

fitzgen merged PR #7593.

Last updated: Dec 23 2024 at 12:05 UTC