Skip to content

[RISCV][GISel] Add manual isel for s16 load/store for the GPR bank. #116111

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

Closed
wants to merge 1 commit into from
Closed

[RISCV][GISel] Add manual isel for s16 load/store for the GPR bank. #116111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
65 changes: 65 additions & 0 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 @@ -580,6 +580,23 @@ static void getOperandsForBranch(Register CondReg, RISCVCC::CondCode &CC,
CC = getRISCVCCFromICmp(Pred);
}

/// Select the RISC-V opcode for the G_LOAD or G_STORE operation \p GenericOpc,
/// appropriate for the (value) register bank \p RegBankID and of memory access
/// size \p OpSize.
/// \returns \p GenericOpc if the combination is unsupported.
static unsigned selectLoadStoreOp(unsigned GenericOpc, unsigned RegBankID,
unsigned OpSize) {
const bool IsStore = GenericOpc == TargetOpcode::G_STORE;
if (RegBankID == RISCV::GPRBRegBankID) {
switch (OpSize) {
case 16:
return IsStore ? RISCV::SH : RISCV::LH;
}
}

return GenericOpc;
}

bool RISCVInstructionSelector::select(MachineInstr &MI) {
MachineBasicBlock &MBB = *MI.getParent();
MachineFunction &MF = *MBB.getParent();
Expand Down Expand Up @@ -786,6 +803,54 @@ bool RISCVInstructionSelector::select(MachineInstr &MI) {
return selectMergeValues(MI, MIB);
case TargetOpcode::G_UNMERGE_VALUES:
return selectUnmergeValues(MI, MIB);
case TargetOpcode::G_LOAD:
case TargetOpcode::G_STORE: {
GLoadStore &LdSt = cast<GLoadStore>(MI);
LLT PtrTy = MRI->getType(LdSt.getPointerReg());

if (PtrTy != LLT::pointer(0, STI.getXLen())) {
LLVM_DEBUG(dbgs() << "Load/Store pointer has type: " << PtrTy
<< ", expected: " << LLT::pointer(0, STI.getXLen())
<< '\n');
return false;
}

#ifndef NDEBUG
const RegisterBank &PtrRB =
*RBI.getRegBank(LdSt.getPointerReg(), *MRI, TRI);
// Check that the pointer register is valid.
assert(PtrRB.getID() == RISCV::GPRBRegBankID &&
"Load/Store pointer operand isn't a GPR");
Comment on lines +819 to +823
Copy link
Contributor

Choose a reason for hiding this comment

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

We really need a regbank legality verifier

#endif

unsigned MemSizeInBits = LdSt.getMemSizeInBits().getValue();

const Register ValReg = LdSt.getReg(0);
const RegisterBank &RB = *RBI.getRegBank(ValReg, *MRI, TRI);

const unsigned NewOpc =
selectLoadStoreOp(MI.getOpcode(), RB.getID(), MemSizeInBits);
if (NewOpc == MI.getOpcode())
return false;

// Check if we can fold anything into the addressing mode.
auto AddrModeFns = selectAddrRegImm(MI.getOperand(1));
if (!AddrModeFns)
return false;

// Folded something. Create a new instruction and return it.
auto NewInst = MIB.buildInstr(NewOpc, {}, {}, MI.getFlags());
if (isa<GStore>(MI))
NewInst.addUse(ValReg);
else
NewInst.addDef(ValReg);
NewInst.cloneMemRefs(MI);
for (auto &Fn : *AddrModeFns)
Fn(NewInst);
MI.eraseFromParent();

return constrainSelectedInstRegOperands(*NewInst, TII, TRI, RBI);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be selectable from patterns

Copy link
Collaborator Author

@topperc topperc Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How? i16 isn't in the GPR register class.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can just add it to the class definition. If you're trying to do this it should be there. The set of dag legal types is not influenced by the tablegen class definition

Copy link
Collaborator Author

@topperc topperc Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Weirdly the types in the register class influences something in how tablegen generates the SelectiondDAG output patterns for patterns with multiple instructions. Ever since I added i32 to GPR on RV64 we've been fighting with this #81192 Maybe it's a weird interaction with HwMode.

Copy link
Contributor

Choose a reason for hiding this comment

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

The order of the types in the class is significant in tablegen, for some reason. If you order it last, it might be less trouble

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 I've traced it back to ForceArbitraryInstResultType. Which just picks the first type in the set. Which is probably ordered by the MVT enum.

/// Given a pattern result with an unresolved type, see if we can find one       
/// instruction with an unresolved result type.  Force this result type to an    
/// arbitrary element if it's possible types to converge results.                
static bool ForceArbitraryInstResultType(TreePatternNode &N, TreePattern &TP) {

}
default:
return false;
}
Expand Down
25 changes: 24 additions & 1 deletion llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/load-rv32.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
# RUN: llc -mtriple=riscv32 -run-pass=instruction-select %s -o - \
# RUN: | FileCheck %s
# RUN: -disable-gisel-legality-check | FileCheck %s

---
name: load_i8
Expand Down Expand Up @@ -45,6 +45,29 @@ body: |
$x10 = COPY %1(s32)
PseudoRET implicit $x10

...
---
name: load_i16_i16
legalized: true
regBankSelected: true
tracksRegLiveness: true
body: |
bb.0:
liveins: $x10

; CHECK-LABEL: name: load_i16_i16
; CHECK: liveins: $x10
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr = COPY $x10
; CHECK-NEXT: [[LH:%[0-9]+]]:gpr = LH [[COPY]], 0 :: (load (s16))
; CHECK-NEXT: $x10 = COPY [[LH]]
; CHECK-NEXT: PseudoRET implicit $x10
%0:gprb(p0) = COPY $x10
%1:gprb(s16) = G_LOAD %0(p0) :: (load (s16))
%2:gprb(s32) = G_ANYEXT %1
$x10 = COPY %2(s32)
PseudoRET implicit $x10

...
---
name: load_i32
Expand Down
25 changes: 24 additions & 1 deletion llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/load-rv64.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
# RUN: llc -mtriple=riscv64 -run-pass=instruction-select %s -o - \
# RUN: | FileCheck %s
# RUN: -disable-gisel-legality-check | FileCheck %s

---
name: load_i8_i64
Expand Down Expand Up @@ -45,6 +45,29 @@ body: |
$x10 = COPY %1(s64)
PseudoRET implicit $x10

...
---
name: load_i16_i16
legalized: true
regBankSelected: true
tracksRegLiveness: true
body: |
bb.0:
liveins: $x10

; CHECK-LABEL: name: load_i16_i16
; CHECK: liveins: $x10
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr = COPY $x10
; CHECK-NEXT: [[LH:%[0-9]+]]:gpr = LH [[COPY]], 0 :: (load (s16))
; CHECK-NEXT: $x10 = COPY [[LH]]
; CHECK-NEXT: PseudoRET implicit $x10
%0:gprb(p0) = COPY $x10
%1:gprb(s16) = G_LOAD %0(p0) :: (load (s16))
%2:gprb(s64) = G_ANYEXT %1
$x10 = COPY %2(s64)
PseudoRET implicit $x10

...
---
name: load_i32_i64
Expand Down
25 changes: 24 additions & 1 deletion llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/store-rv32.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
# RUN: llc -mtriple=riscv32 -run-pass=instruction-select %s -o - \
# RUN: | FileCheck %s
# RUN: -disable-gisel-legality-check | FileCheck %s
#
---
name: store_i8
Expand Down Expand Up @@ -45,6 +45,29 @@ body: |
G_STORE %0(s32), %1(p0) :: (store (s16))
PseudoRET

...
---
name: store_i16_i16
legalized: true
regBankSelected: true
tracksRegLiveness: true
body: |
bb.0:
liveins: $x10, $x11

; CHECK-LABEL: name: store_i16_i16
; CHECK: liveins: $x10, $x11
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr = COPY $x10
; CHECK-NEXT: [[COPY1:%[0-9]+]]:gpr = COPY $x11
; CHECK-NEXT: SH [[COPY]], [[COPY1]], 0 :: (store (s16))
; CHECK-NEXT: PseudoRET
%0:gprb(s32) = COPY $x10
%1:gprb(p0) = COPY $x11
%2:gprb(s16) = G_TRUNC %0
G_STORE %2(s16), %1(p0) :: (store (s16))
PseudoRET

...
---
name: store_i32
Expand Down
25 changes: 24 additions & 1 deletion llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/store-rv64.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
# RUN: llc -mtriple=riscv64 -run-pass=instruction-select %s -o - \
# RUN: | FileCheck %s
# RUN: -disable-gisel-legality-check | FileCheck %s

---
name: store_i8_i64
Expand Down Expand Up @@ -45,6 +45,29 @@ body: |
G_STORE %0(s64), %1(p0) :: (store (s16))
PseudoRET

...
---
name: store_i16_i16
legalized: true
regBankSelected: true
tracksRegLiveness: true
body: |
bb.0:
liveins: $x10, $x11

; CHECK-LABEL: name: store_i16_i16
; CHECK: liveins: $x10, $x11
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr = COPY $x10
; CHECK-NEXT: [[COPY1:%[0-9]+]]:gpr = COPY $x11
; CHECK-NEXT: SH [[COPY]], [[COPY1]], 0 :: (store (s16))
; CHECK-NEXT: PseudoRET
%0:gprb(s64) = COPY $x10
%1:gprb(p0) = COPY $x11
%2:gprb(s16) = G_TRUNC %0
G_STORE %2(s16), %1(p0) :: (store (s16))
PseudoRET

...
---
name: store_i32_i64
Expand Down
Loading