Stream: git-wasmtime

Topic: wasmtime / PR #5487 Cranelift: fix branch-of-icmp/fcmp re...

Wasmtime GitHub notifications bot (Dec 22 2022 at 06:51):

cfallin opened PR #5487 from fix-branch-of-uextend-of-icmp to main:

In #5031, we removed bool types from CLIF, using integers instead for "truthy" values. This greatly simplified the IR, and was generally an improvement.

However, because x86's SETcc instruction sets only the low 8 bits of a register, we chose to use i8 types as the result of icmp and fcmp, to avoid the need for a masking operation when materializing the result.

Unfortunately this means that uses of truthy values often now have uextend operations, especially when coming from Wasm (where truthy values are naturally i32-typed). For example, where we previously had (brz (icmp ...)), we now have (brz (uextend (icmp ...))).

It's arguable whether or not we should switch to i32 truthy values -- in most cases we can avoid materializing a value that's immediately used for a branch or select, so a mask would in most cases be unnecessary, and it would be a win at the IR level -- but irrespective of that, this change did regress our generated code quality: our backends had patterns for e.g. (brz (icmp ...)) but not with the uextend, so we were always materializing truthy values. Many blocks thus ended with "cmp; setcc; cmp; test; branch" rather than "cmp; branch".

In #5391 we noticed this and fixed it on x64, but it was a general problem on aarch64 and riscv64 as well. This PR introduces a maybe_uextend extractor that "looks through" uextends, and uses it where we consume truthy values, thus fixing the regression. This PR also adds compile filetests to ensure we don't regress again.

<!--

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 (Dec 22 2022 at 06:51):

cfallin requested jameysharp for a review on PR #5487.

Wasmtime GitHub notifications bot (Dec 22 2022 at 08:23):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 22 2022 at 08:57):

cfallin updated PR #5487 from fix-branch-of-uextend-of-icmp to main.

Wasmtime GitHub notifications bot (Dec 22 2022 at 09:43):

cfallin merged PR #5487.

Last updated: Dec 23 2024 at 12:05 UTC