Wasmtime GitHub notifications bot (May 25 2023 at 18:40): Wasmtime GitHub notifications bot (May 25 2023 at 18:40):afonso360 opened PR #6459 from afonso360:riscv-reg-newtypes to bytecodealliance:main:
:wave: Hey,
This PR is a followup to a suggestion made by @alexcrichton, it adds newtype wrappers for the different register classes that we have in the RISC-V Backend.
These are
XReg,FRegandVReg, for Integer, Float and Vector. The newtypes are contained only to the ISLE layer, and do not propagate throughout the rest of the backend. However It would be a good idea to iteratively do so, I just felt that it would be too big a step for a first PR.This change was further complicated by a rule that we have in the backend where we auto convert
ValueRegsintoReg. This rule has caused issues in the past (1, 2), and was also conflicting with the newtype wrappers. Instead of adding similar rules to the newtypes, I've removed the rule and all usages of it.Most of this change was in
ext_int_if_needwhich returns ValueRegs, but most of its uses don't actually need more than one register. To that end I've repurposed thezextandsextrules, to act as single register zero or sign extends, and replaced the uses where only one register was necessary with those.These changes haven't changed any of the golden lowerings, but since it's such a broad change that I suspect we might be missing some coverage and I'm going to be fuzzing this overnight to doublecheck.
Wasmtime GitHub notifications bot (May 25 2023 at 18:40):afonso360 requested wasmtime-compiler-reviewers for a review on PR #6459.
Wasmtime GitHub notifications bot (May 25 2023 at 18:40):afonso360 requested fitzgen for a review on PR #6459.
Wasmtime GitHub notifications bot (May 25 2023 at 18:41):afonso360 edited PR #6459:
:wave: Hey,
This PR is a followup to a suggestion made by @alexcrichton, it adds newtype wrappers for the different register classes that we have in the RISC-V Backend.
These are
XReg,FRegandVReg, for Integer, Float and Vector. The newtypes are contained only to the ISLE layer, and do not propagate throughout the rest of the backend. However It would be a good idea to iteratively do so, I just felt that it would be too big a step for a first PR.This change was further complicated by a rule that we have in the backend where we auto convert
ValueRegsintoReg. This rule has caused issues in the past (1, 2), and was also conflicting with the newtype wrappers. Instead of adding similar rules to the newtypes, I've removed the rule and all usages of it.Most of this change was in
ext_int_if_needwhich returns ValueRegs, but most of its uses don't actually need more than one register. To that end I've repurposed thezextandsextrules, to act as single register zero or sign extends, and replaced the uses where only one register was necessary with those. Extends that do need multiple registers can useext_int_if_needorextend.These changes haven't changed any of the golden lowerings, but since it's such a broad change that I suspect we might be missing some coverage and I'm going to be fuzzing this overnight to doublecheck.
Wasmtime GitHub notifications bot (May 25 2023 at 18:43):afonso360 edited PR #6459:
:wave: Hey,
This PR is a followup to a suggestion made by @alexcrichton, it adds newtype wrappers for the different register classes that we have in the RISC-V Backend.
These are
XReg,FRegandVReg, for Integer, Float and Vector. The newtypes are contained only to the ISLE layer, and do not propagate throughout the rest of the backend. However It would be a good idea to iteratively do so, I just felt that it would be too big a step for a first PR.This change was further complicated by a rule that we have in the backend where we auto convert
ValueRegsintoReg. This rule has caused issues in the past (1, 2), and was also conflicting with the newtype wrappers. Instead of adding similar rules to the newtypes, I've removed the rule and all usages of it.Most of this change was in
ext_int_if_needwhich returns ValueRegs, but most of its uses don't actually need more than one register. To that end I've repurposed thezextandsextrules, to act as single register zero or sign extends, and replaced the uses where only one register was necessary with those. Extends that do need multiple registers can useext_int_if_needorextend.These changes haven't changed any of the golden lowerings, but since it's such a broad change, I suspect we might be missing some coverage and I'm going to be fuzzing this overnight to doublecheck.
Wasmtime GitHub notifications bot (May 25 2023 at 19:50):fitzgen submitted PR review:
Nice!
Wasmtime GitHub notifications bot (May 25 2023 at 19:51):fitzgen has enabled auto merge for PR #6459.
Wasmtime GitHub notifications bot (May 25 2023 at 20:18):afonso360 updated PR #6459.
Wasmtime GitHub notifications bot (May 25 2023 at 20:19):afonso360 has disabled auto merge for PR #6459.
Wasmtime GitHub notifications bot (May 26 2023 at 15:18):afonso360 merged PR #6459.
Last updated: Dec 23 2024 at 13:07 UTC