Skip to content

[RISCV] Fold addi into load / store even if they are in different BBs. #67024

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions llvm/lib/Target/RISCV/RISCVISelLowering.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ using namespace llvm;
#define DEBUG_TYPE "riscv-lower"

STATISTIC(NumTailCalls, "Number of tail calls");
STATISTIC(NumADDIsMerged, "Number of ADDIs merged.");

static cl::opt<unsigned> ExtensionMaxWebSize(
DEBUG_TYPE "-ext-max-web-size", cl::Hidden,
Expand Down Expand Up @@ -2278,6 +2279,81 @@ bool RISCVTargetLowering::isLegalElementTypeForRVV(EVT ScalarTy) const {
}
}

static bool tryToFoldInstIntoUse(MachineInstr &UseMI, MachineInstr &MI) {

if (MI.getOpcode() != RISCV::ADDI)
return false;
if (!(MI.getOperand(0).isReg() && MI.getOperand(1).isReg()))
return false;

switch (UseMI.getOpcode()) {
default:
return false;
case RISCV::LB:
case RISCV::LH:
case RISCV::LW:
case RISCV::LD:
case RISCV::LBU:
case RISCV::LHU:
case RISCV::SB:
case RISCV::SH:
case RISCV::SW:
case RISCV::SD:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized I should also include FP loads and stores.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're also missing LWU

break;
}
MachineOperand &OriginalBaseMO = UseMI.getOperand(1);
if (!OriginalBaseMO.isReg())
return false;
if (OriginalBaseMO.getReg() != MI.getOperand(0).getReg())
return false;

MachineOperand &OriginalOffsetMO = UseMI.getOperand(2);
MachineOperand &ADDIOffsetMO = MI.getOperand(2);
if (!(OriginalOffsetMO.isImm() && ADDIOffsetMO.isImm()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be an assert since we know this instruction is an ADDI?

Copy link
Contributor Author

@mgudim mgudim Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be a symbol? Like something that only gets resolved by linker?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it can be a symbol. We do need to check isImm().

return false;

int64_t OriginalOffset = OriginalOffsetMO.getImm();
int64_t ADDIOffset = ADDIOffsetMO.getImm();
int64_t TotalOffset = OriginalOffset + ADDIOffset;
if (!isInt<12>(TotalOffset))
return false;

OriginalOffsetMO.setImm(TotalOffset);
OriginalBaseMO.setReg(MI.getOperand(1).getReg());
NumADDIsMerged++;
return true;
}

void RISCVTargetLowering::finalizeLowering(MachineFunction &MF) const {
TargetLoweringBase::finalizeLowering(MF);
MachineRegisterInfo &MRI = MF.getRegInfo();

SmallVector<MachineInstr *, 8> ToErase;
for (MachineFunction::iterator I = MF.begin(), E = MF.end(); I != E; ++I) {
MachineBasicBlock *MBB = &*I;
for (MachineBasicBlock::iterator MBBI = MBB->begin(), MBBE = MBB->end();
MBBI != MBBE;) {
MachineInstr &MI = *MBBI++;
if (MI.getOpcode() != RISCV::ADDI)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you already check Opcode == ADDI and Operand0.isReg() in tryToFoldInstIntoUse

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, thanks!

continue;
if (!MI.getOperand(0).isReg())
continue;
SmallVector<MachineInstr *, 4> Users;
for (MachineInstr &UseMI :
MRI.use_instructions(MI.getOperand(0).getReg()))
Users.push_back(&UseMI);
bool AllUsesWereFolded = true;
for (MachineInstr *UseMI : Users)
AllUsesWereFolded &= tryToFoldInstIntoUse(*UseMI, MI);
if (AllUsesWereFolded)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all uses aren't folded, then this would increase register pressure wouldn't it?

ToErase.push_back(&MI);
}
}
for (MachineInstr *MI : ToErase)
MI->eraseFromParent();

return;
}

unsigned RISCVTargetLowering::combineRepeatedFPDivisors() const {
return NumRepeatedDivisors;
Expand Down
2 changes: 2 additions & 0 deletions llvm/lib/Target/RISCV/RISCVISelLowering.h
Original file line number Diff line number Diff line change
Expand Up @@ -951,6 +951,8 @@ class RISCVTargetLowering : public TargetLowering {
return false;
};

void finalizeLowering(MachineFunction &MF) const override;

/// For available scheduling models FDIV + two independent FMULs are much
/// faster than two FDIVs.
unsigned combineRepeatedFPDivisors() const override;
Expand Down