Conversation
…es into canonicalizeWSuffixes This refactor was suggested in <llvm#144703>. I have checked for unexpected changes by comparing builds of llvm-test-suite with/without this refactor, including with preferWInst force enabled.
Member
|
@llvm/pr-subscribers-backend-risc-v Author: Alex Bradbury (asb) ChangesThis refactor was suggested in I have checked for unexpected changes by comparing builds of llvm-test-suite with/without this refactor, including with preferWInst force enabled. After this, the logic changes for #144703 can be implemented with just: --- a/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp
+++ b/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp
@@ -736,7 +736,8 @@ bool RISCVOptWInstrs::canonicalizeWSuffixes(MachineFunction &MF,
for (MachineInstr &MI : MBB) {
std::optional<unsigned> WOpc;
std::optional<unsigned> NonWOpc;
- switch (MI.getOpcode()) {
+ unsigned OrigOpc = MI.getOpcode();
+ switch (OrigOpc) {
default:
continue;
case RISCV::ADDW:
@@ -786,7 +787,8 @@ bool RISCVOptWInstrs::canonicalizeWSuffixes(MachineFunction &MF,
MadeChange = true;
continue;
}
- if (ShouldPreferW && WOpc.has_value() && hasAllWUsers(MI, ST, MRI)) {
+ if ((ShouldPreferW || OrigOpc == RISCV::LWU) && WOpc.has_value() &&
+ hasAllWUsers(MI, ST, MRI)) {
LLVM_DEBUG(dbgs() << "Replacing " << MI);
MI.setDesc(TII.get(WOpc.value()));
MI.clearFlag(MachineInstr::MIFlag::NoSWrap);Full diff: https://github.com/llvm/llvm-project/pull/149710.diff 1 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp b/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp
index 09a31fb2306de..35e0f733062f0 100644
--- a/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp
+++ b/llvm/lib/Target/RISCV/RISCVOptWInstrs.cpp
@@ -69,10 +69,9 @@ class RISCVOptWInstrs : public MachineFunctionPass {
bool runOnMachineFunction(MachineFunction &MF) override;
bool removeSExtWInstrs(MachineFunction &MF, const RISCVInstrInfo &TII,
const RISCVSubtarget &ST, MachineRegisterInfo &MRI);
- bool stripWSuffixes(MachineFunction &MF, const RISCVInstrInfo &TII,
- const RISCVSubtarget &ST, MachineRegisterInfo &MRI);
- bool appendWSuffixes(MachineFunction &MF, const RISCVInstrInfo &TII,
- const RISCVSubtarget &ST, MachineRegisterInfo &MRI);
+ bool canonicalizeWSuffixes(MachineFunction &MF, const RISCVInstrInfo &TII,
+ const RISCVSubtarget &ST,
+ MachineRegisterInfo &MRI);
void getAnalysisUsage(AnalysisUsage &AU) const override {
AU.setPreservesCFG();
@@ -723,51 +722,38 @@ bool RISCVOptWInstrs::removeSExtWInstrs(MachineFunction &MF,
return MadeChange;
}
-bool RISCVOptWInstrs::stripWSuffixes(MachineFunction &MF,
- const RISCVInstrInfo &TII,
- const RISCVSubtarget &ST,
- MachineRegisterInfo &MRI) {
+// Strips or adds W suffixes to eligible instructions depending on the
+// subtarget preferences.
+bool RISCVOptWInstrs::canonicalizeWSuffixes(MachineFunction &MF,
+ const RISCVInstrInfo &TII,
+ const RISCVSubtarget &ST,
+ MachineRegisterInfo &MRI) {
+ bool ShouldStripW = !(DisableStripWSuffix || ST.preferWInst());
+ bool ShouldPreferW = ST.preferWInst();
bool MadeChange = false;
- for (MachineBasicBlock &MBB : MF) {
- for (MachineInstr &MI : MBB) {
- unsigned Opc;
- // clang-format off
- switch (MI.getOpcode()) {
- default:
- continue;
- case RISCV::ADDW: Opc = RISCV::ADD; break;
- case RISCV::ADDIW: Opc = RISCV::ADDI; break;
- case RISCV::MULW: Opc = RISCV::MUL; break;
- case RISCV::SLLIW: Opc = RISCV::SLLI; break;
- case RISCV::SUBW: Opc = RISCV::SUB; break;
- }
- // clang-format on
- if (hasAllWUsers(MI, ST, MRI)) {
- LLVM_DEBUG(dbgs() << "Replacing " << MI);
- MI.setDesc(TII.get(Opc));
- LLVM_DEBUG(dbgs() << " with " << MI);
- ++NumTransformedToNonWInstrs;
- MadeChange = true;
- }
- }
- }
-
- return MadeChange;
-}
-
-bool RISCVOptWInstrs::appendWSuffixes(MachineFunction &MF,
- const RISCVInstrInfo &TII,
- const RISCVSubtarget &ST,
- MachineRegisterInfo &MRI) {
- bool MadeChange = false;
for (MachineBasicBlock &MBB : MF) {
for (MachineInstr &MI : MBB) {
- unsigned WOpc;
- // TODO: Add more?
+ std::optional<unsigned> WOpc;
+ std::optional<unsigned> NonWOpc;
switch (MI.getOpcode()) {
default:
continue;
+ case RISCV::ADDW:
+ NonWOpc = RISCV::ADD;
+ break;
+ case RISCV::ADDIW:
+ NonWOpc = RISCV::ADDI;
+ break;
+ case RISCV::MULW:
+ NonWOpc = RISCV::MUL;
+ break;
+ case RISCV::SLLIW:
+ NonWOpc = RISCV::SLLI;
+ break;
+ case RISCV::SUBW:
+ NonWOpc = RISCV::SUB;
+ break;
case RISCV::ADD:
WOpc = RISCV::ADDW;
break;
@@ -781,7 +767,7 @@ bool RISCVOptWInstrs::appendWSuffixes(MachineFunction &MF,
WOpc = RISCV::MULW;
break;
case RISCV::SLLI:
- // SLLIW reads the lowest 5 bits, while SLLI reads lowest 6 bits
+ // SLLIW reads the lowest 5 bits, while SLLI reads lowest 6 bits.
if (MI.getOperand(2).getImm() >= 32)
continue;
WOpc = RISCV::SLLIW;
@@ -792,19 +778,27 @@ bool RISCVOptWInstrs::appendWSuffixes(MachineFunction &MF,
break;
}
- if (hasAllWUsers(MI, ST, MRI)) {
+ if (ShouldStripW && NonWOpc.has_value() && hasAllWUsers(MI, ST, MRI)) {
LLVM_DEBUG(dbgs() << "Replacing " << MI);
- MI.setDesc(TII.get(WOpc));
+ MI.setDesc(TII.get(NonWOpc.value()));
+ LLVM_DEBUG(dbgs() << " with " << MI);
+ ++NumTransformedToNonWInstrs;
+ MadeChange = true;
+ continue;
+ }
+ if (ShouldPreferW && WOpc.has_value() && hasAllWUsers(MI, ST, MRI)) {
+ LLVM_DEBUG(dbgs() << "Replacing " << MI);
+ MI.setDesc(TII.get(WOpc.value()));
MI.clearFlag(MachineInstr::MIFlag::NoSWrap);
MI.clearFlag(MachineInstr::MIFlag::NoUWrap);
MI.clearFlag(MachineInstr::MIFlag::IsExact);
LLVM_DEBUG(dbgs() << " with " << MI);
++NumTransformedToWInstrs;
MadeChange = true;
+ continue;
}
}
}
-
return MadeChange;
}
@@ -821,12 +815,6 @@ bool RISCVOptWInstrs::runOnMachineFunction(MachineFunction &MF) {
bool MadeChange = false;
MadeChange |= removeSExtWInstrs(MF, TII, ST, MRI);
-
- if (!(DisableStripWSuffix || ST.preferWInst()))
- MadeChange |= stripWSuffixes(MF, TII, ST, MRI);
-
- if (ST.preferWInst())
- MadeChange |= appendWSuffixes(MF, TII, ST, MRI);
-
+ MadeChange |= canonicalizeWSuffixes(MF, TII, ST, MRI);
return MadeChange;
}
|
asb
changed the title
wangpc-pp
approved these changes
Jul 21, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/23763 Here is the relevant piece of the build log for the reference |
asb
added a commit
that referenced
this pull request
Jul 21, 2025
After the refactoring in #149710 the logic change is trivial. Motivation for preferring sign-extended 32-bit loads (LW) vs zero-extended (LWU): * LW is compressible while LWU is not. * Helps to minimise the diff vs RV32 (e.g. LWU vs LW) * Helps to minimise distracting diffs vs GCC. I see this come up frequently when comparing GCC code and in these cases it's a red herring. Similar normalisation could be done for LHU and LH, but this is less well motivated as there is a compressed LHU (and if performing the change in RISCVOptWInstrs it wouldn't be done for RV32). There is a compressed LBU but not LB, meaning doing a similar normalisation for byte-sized loads would actually be a regression in terms of code size. Load narrowing when allowed by hasAllNBitUsers isn't explored in this patch. This changes ~20500 instructions in an RVA22 build of the llvm-test-suite including SPEC 2017. As part of the review, the option of doing the change at ISel time was explored but was found to be less effective.
This was referenced Jul 23, 2025
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Jul 28, 2025
…xes into canonicalizeWSuffixes (llvm#149710) This refactor was suggested in <llvm#144703>. I have checked for unexpected changes by comparing builds of llvm-test-suite with/without this refactor, including with preferWInst force enabled.
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Jul 28, 2025
After the refactoring in llvm#149710 the logic change is trivial. Motivation for preferring sign-extended 32-bit loads (LW) vs zero-extended (LWU): * LW is compressible while LWU is not. * Helps to minimise the diff vs RV32 (e.g. LWU vs LW) * Helps to minimise distracting diffs vs GCC. I see this come up frequently when comparing GCC code and in these cases it's a red herring. Similar normalisation could be done for LHU and LH, but this is less well motivated as there is a compressed LHU (and if performing the change in RISCVOptWInstrs it wouldn't be done for RV32). There is a compressed LBU but not LB, meaning doing a similar normalisation for byte-sized loads would actually be a regression in terms of code size. Load narrowing when allowed by hasAllNBitUsers isn't explored in this patch. This changes ~20500 instructions in an RVA22 build of the llvm-test-suite including SPEC 2017. As part of the review, the option of doing the change at ISel time was explored but was found to be less effective.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This refactor was suggested in
#144703.
I have checked for unexpected changes by comparing builds of llvm-test-suite with/without this refactor, including with preferWInst force enabled.
After this, the logic changes for #144703 can be implemented with just: