Stream: git-wasmtime

Topic: wasmtime / PR #5512 riscv64: Implement fcmp in ISLE


view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 07:03):

elliottt edited PR #5512 from trevor/riscv64-br-fcmp to main:

<!--

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

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

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 07:08):

elliottt edited PR #5512 from trevor/riscv64-br-fcmp to main:

Rework the compilation of fcmp in the riscv64 backend to be in ISLE, removing the need for the dedicated Fcmp instruction. This change is motivated by #5500, which showed that the riscv64 backend was generating branch instructions in the middle of a basic block.

Fixes #5500

<!--

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

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

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 08:20):

elliottt updated PR #5512 from trevor/riscv64-br-fcmp to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 08:21):

elliottt edited PR #5512 from trevor/riscv64-br-fcmp to main:

Rework the compilation of fcmp in the riscv64 backend to be in ISLE, removing the need for the dedicated Fcmp instruction. This change is motivated by #5500, which showed that the riscv64 backend was generating branch instructions in the middle of a basic block.

We can't quite remove lower_br_fcmp yet as it's used in a few places in the emit module, but it's now no longer reachable from the ISLE lowerings.

Fixes #5500

<!--

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

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

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 08:21):

elliottt edited PR #5512 from trevor/riscv64-br-fcmp to main:

Rework the compilation of fcmp in the riscv64 backend to be in ISLE, removing the need for the dedicated Fcmp instruction. This change is motivated by #5500, which showed that the riscv64 backend was generating branch instructions in the middle of a basic block.

We can't remove lower_br_fcmp quite yet as it's used in a few places in the emit module, but it's now no longer reachable from the ISLE lowerings.

Fixes #5500

<!--

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

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

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 17:06):

elliottt updated PR #5512 from trevor/riscv64-br-fcmp to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 17:58):

elliottt has marked PR #5512 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 18:00):

elliottt requested jameysharp for a review on PR #5512.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 18:33):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 18:33):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 18:33):

jameysharp created PR review comment:

;; Construct an IntegerCompare value.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 18:33):

jameysharp created PR review comment:

This comment should say a == b rather than a < b, I think.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 18:33):

jameysharp created PR review comment:

Was this change supposed to be in this PR? It looks like it's a part of #5515 that leaked into here.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 18:45):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 18:45):

elliottt created PR review comment:

Thanks for catching that! Originally I was trying to remove lower_br_fcmp in this PR, but started feeling like it was becoming a bit unwieldy.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 18:48):

elliottt updated PR #5512 from trevor/riscv64-br-fcmp to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 18:58):

elliottt updated PR #5512 from trevor/riscv64-br-fcmp to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2023 at 19:52):

elliottt merged PR #5512.


Last updated: Dec 23 2024 at 12:05 UTC