Skip to content

[RISCV][GISel] Move G_BRJT expansion to legalization #73711

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Nov 20, 2024
Merged

[RISCV][GISel] Move G_BRJT expansion to legalization #73711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
faac67b
[RISCV][GISel] Move G_BRJT expansion to legalization
topperc Nov 28, 2023
2f83efb
fixup! reuse IndexReg.
topperc Nov 29, 2023
fd463cf
fixup! return false for unknown entry kind
topperc Nov 29, 2023
e67d2dd
fixup! use early out
topperc Nov 29, 2023
33200b5
Merge remote-tracking branch 'origin/main' into pr/brjt-legalize
topperc Nov 20, 2024
868efa3
fixup! Use G_SEXTLOAD for PIC on RV64.
topperc Nov 20, 2024
f98a828
fixup! clang-format
topperc Nov 20, 2024
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
54 changes: 0 additions & 54 deletions llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,8 +581,6 @@ static void getOperandsForBranch(Register CondReg, RISCVCC::CondCode &CC,
}

bool RISCVInstructionSelector::select(MachineInstr &MI) {
MachineBasicBlock &MBB = *MI.getParent();
MachineFunction &MF = *MBB.getParent();
MachineIRBuilder MIB(MI);

preISelLower(MI, MIB);
Expand Down Expand Up @@ -703,58 +701,6 @@ bool RISCVInstructionSelector::select(MachineInstr &MI) {
MI.eraseFromParent();
return constrainSelectedInstRegOperands(*Bcc, TII, TRI, RBI);
}
case TargetOpcode::G_BRJT: {
// FIXME: Move to legalization?
const MachineJumpTableInfo *MJTI = MF.getJumpTableInfo();
unsigned EntrySize = MJTI->getEntrySize(MF.getDataLayout());
assert((EntrySize == 4 || (Subtarget->is64Bit() && EntrySize == 8)) &&
"Unsupported jump-table entry size");
assert(
(MJTI->getEntryKind() == MachineJumpTableInfo::EK_LabelDifference32 ||
MJTI->getEntryKind() == MachineJumpTableInfo::EK_Custom32 ||
MJTI->getEntryKind() == MachineJumpTableInfo::EK_BlockAddress) &&
"Unexpected jump-table entry kind");

auto SLL =
MIB.buildInstr(RISCV::SLLI, {&RISCV::GPRRegClass}, {MI.getOperand(2)})
.addImm(Log2_32(EntrySize));
if (!SLL.constrainAllUses(TII, TRI, RBI))
return false;

// TODO: Use SHXADD. Moving to legalization would fix this automatically.
auto ADD = MIB.buildInstr(RISCV::ADD, {&RISCV::GPRRegClass},
{MI.getOperand(0), SLL.getReg(0)});
if (!ADD.constrainAllUses(TII, TRI, RBI))
return false;

unsigned LdOpc = EntrySize == 8 ? RISCV::LD : RISCV::LW;
auto Dest =
MIB.buildInstr(LdOpc, {&RISCV::GPRRegClass}, {ADD.getReg(0)})
.addImm(0)
.addMemOperand(MF.getMachineMemOperand(
MachinePointerInfo::getJumpTable(MF), MachineMemOperand::MOLoad,
EntrySize, Align(MJTI->getEntryAlignment(MF.getDataLayout()))));
if (!Dest.constrainAllUses(TII, TRI, RBI))
return false;

// If the Kind is EK_LabelDifference32, the table stores an offset from
// the location of the table. Add the table address to get an absolute
// address.
if (MJTI->getEntryKind() == MachineJumpTableInfo::EK_LabelDifference32) {
Dest = MIB.buildInstr(RISCV::ADD, {&RISCV::GPRRegClass},
{Dest.getReg(0), MI.getOperand(0)});
if (!Dest.constrainAllUses(TII, TRI, RBI))
return false;
}

auto Branch =
MIB.buildInstr(RISCV::PseudoBRIND, {}, {Dest.getReg(0)}).addImm(0);
if (!Branch.constrainAllUses(TII, TRI, RBI))
return false;

MI.eraseFromParent();
return true;
}
case TargetOpcode::G_BRINDIRECT:
MI.setDesc(TII.get(RISCV::PseudoBRIND));
MI.addOperand(MachineOperand::CreateImm(0));
Expand Down
60 changes: 59 additions & 1 deletion llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "llvm/CodeGen/GlobalISel/LegalizerHelper.h"
#include "llvm/CodeGen/GlobalISel/MachineIRBuilder.h"
#include "llvm/CodeGen/MachineConstantPool.h"
#include "llvm/CodeGen/MachineJumpTableInfo.h"
#include "llvm/CodeGen/MachineMemOperand.h"
#include "llvm/CodeGen/MachineRegisterInfo.h"
#include "llvm/CodeGen/TargetOpcodes.h"
Expand Down Expand Up @@ -406,7 +407,7 @@ RISCVLegalizerInfo::RISCVLegalizerInfo(const RISCVSubtarget &ST)

getActionDefinitionsBuilder(G_BRCOND).legalFor({sXLen}).minScalar(0, sXLen);

getActionDefinitionsBuilder(G_BRJT).legalFor({{p0, sXLen}});
getActionDefinitionsBuilder(G_BRJT).customFor({{p0, sXLen}});

getActionDefinitionsBuilder(G_BRINDIRECT).legalFor({p0});

Expand Down Expand Up @@ -687,6 +688,61 @@ bool RISCVLegalizerInfo::legalizeVAStart(MachineInstr &MI,
return true;
}

bool RISCVLegalizerInfo::legalizeBRJT(MachineInstr &MI,
MachineIRBuilder &MIRBuilder) const {
MachineRegisterInfo &MRI = *MIRBuilder.getMRI();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can all of this except the Custom32 case go into the generic legalizer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the EK_LabelDifference32 case might not be generic either. SelectionDAG calls a virtual function getPICJumpTableRelocBase.

From LegalizeDAG.cpp

    if (TLI.isJumpTableRelative()) {                                             
      // For PIC, the sequence is:                                               
      // BRIND(load(Jumptable + index) + RelocBase)                              
      // RelocBase can be JumpTable, GOT or some sort of global base.            
      Addr = DAG.getNode(ISD::ADD, dl, PTy, Addr,                                
                          TLI.getPICJumpTableRelocBase(Table, DAG));             
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

For that kind of case I think we could just introduce a global isel variant of the same hook. It still saves a lot of common boilerplate

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we take this patch as is and I'll work on a refactoring into the legalizer as a follow up?

auto &MF = *MI.getParent()->getParent();
const MachineJumpTableInfo *MJTI = MF.getJumpTableInfo();
unsigned EntrySize = MJTI->getEntrySize(MF.getDataLayout());

Register PtrReg = MI.getOperand(0).getReg();
LLT PtrTy = MRI.getType(PtrReg);
Register IndexReg = MI.getOperand(2).getReg();
LLT IndexTy = MRI.getType(IndexReg);

if (!isPowerOf2_32(EntrySize))
return false;

auto ShiftAmt = MIRBuilder.buildConstant(IndexTy, Log2_32(EntrySize));
IndexReg = MIRBuilder.buildShl(IndexTy, IndexReg, ShiftAmt).getReg(0);

auto Addr = MIRBuilder.buildPtrAdd(PtrTy, PtrReg, IndexReg);

MachineMemOperand *MMO = MF.getMachineMemOperand(
MachinePointerInfo::getJumpTable(MF), MachineMemOperand::MOLoad,
EntrySize, Align(MJTI->getEntryAlignment(MF.getDataLayout())));

Register TargetReg;
switch (MJTI->getEntryKind()) {
default:
return false;
case MachineJumpTableInfo::EK_LabelDifference32: {
// For PIC, the sequence is:
// BRIND(load(Jumptable + index) + RelocBase)
// RelocBase can be JumpTable, GOT or some sort of global base.
unsigned LoadOpc =
STI.is64Bit() ? TargetOpcode::G_SEXTLOAD : TargetOpcode::G_LOAD;
auto Load = MIRBuilder.buildLoadInstr(LoadOpc, IndexTy, Addr, *MMO);
TargetReg = MIRBuilder.buildPtrAdd(PtrTy, PtrReg, Load).getReg(0);
break;
}
case MachineJumpTableInfo::EK_Custom32: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we treat EK_Custom32 differently from EK_BlockAddress here, but didn't have to in instruction selection?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

EK_BlockAddress stores a pointer to the basic block target using a full pointer worth of bits. EK_Custom32 is an RV64 size optimization for the small code model where we assume that the 64 bit pointer is in +/-2GB so fits in 32 bits. Since its 32 bits it's type doesn't match p0(64 bits) so we have to sign extend it and convert it.

In instruction selection, EK_BlockAddress used LW on rv32 and LD on rv64. EK_Custom32 used LW. We distinguished based on the EntrySize.

auto Load = MIRBuilder.buildLoadInstr(TargetOpcode::G_SEXTLOAD, IndexTy,
Addr, *MMO);
TargetReg = MIRBuilder.buildIntToPtr(PtrTy, Load).getReg(0);
break;
}
case MachineJumpTableInfo::EK_BlockAddress:
TargetReg = MIRBuilder.buildLoad(PtrTy, Addr, *MMO).getReg(0);
break;
}

MIRBuilder.buildBrIndirect(TargetReg);

MI.eraseFromParent();
return true;
}

bool RISCVLegalizerInfo::shouldBeInConstantPool(const APInt &APImm,
bool ShouldOptForSize) const {
assert(APImm.getBitWidth() == 32 || APImm.getBitWidth() == 64);
Expand Down Expand Up @@ -1313,6 +1369,8 @@ bool RISCVLegalizerInfo::legalizeCustom(
MI.eraseFromParent();
return true;
}
case TargetOpcode::G_BRJT:
return legalizeBRJT(MI, MIRBuilder);
case TargetOpcode::G_VASTART:
return legalizeVAStart(MI, MIRBuilder);
case TargetOpcode::G_VSCALE:
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class RISCVLegalizerInfo : public LegalizerInfo {
bool legalizeShlAshrLshr(MachineInstr &MI, MachineIRBuilder &MIRBuilder,
GISelChangeObserver &Observer) const;

bool legalizeBRJT(MachineInstr &MI, MachineIRBuilder &MIRBuilder) const;
bool legalizeVAStart(MachineInstr &MI, MachineIRBuilder &MIRBuilder) const;
bool legalizeVScale(MachineInstr &MI, MachineIRBuilder &MIB) const;
bool legalizeExt(MachineInstr &MI, MachineIRBuilder &MIRBuilder) const;
Expand Down
160 changes: 0 additions & 160 deletions llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/jump-table-brjt-medium-rv64.mir
View file
Open in desktop

This file was deleted.

Loading
Loading