Skip to content
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

[RISCV] Split TuneShiftedZExtFusion #76032

Merged
merged 2 commits into from
Dec 22, 2023
Merged

[RISCV] Split TuneShiftedZExtFusion #76032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aaca871
[RISCV] Split TuneShiftedZExtFusion
wangpc-pp Dec 20, 2023
65473de
Rename tests
wangpc-pp Dec 22, 2023
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
16 changes: 13 additions & 3 deletions llvm/lib/Target/RISCV/RISCVFeatures.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -977,9 +977,19 @@ def TuneLUIADDIFusion
def TuneAUIPCADDIFusion
: SubtargetFeature<"auipc-addi-fusion", "HasAUIPCADDIFusion",
"true", "Enable AUIPC+ADDI macrofusion">;
def TuneShiftedZExtFusion
: SubtargetFeature<"shifted-zext-fusion", "HasShiftedZExtFusion",
"true", "Enable SLLI+SRLI to be fused when computing (shifted) zero extension">;

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think "word" is the right word here ) Machine word on riscv64 is 64 bits. How about ZExt16 and ZExt32 instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even if "word" is the right terminology here, I'd still prefer numeric suffixes because they are more explicit

Copy link
Collaborator

Choose a reason for hiding this comment

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

zextw and zexth are consistent with the names of the instructions that were added in Zba and Zbb to replace these shift patterns. But I don't have a strong preference.

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 stand for names zexth and zextw, as @topperc said, they are the names for equivalent existed instructions. I think people may be much more familiar with these. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, sounds good to me. I didn't know about these.

def TuneZExtHFusion
: SubtargetFeature<"zexth-fusion", "HasZExtHFusion",
"true", "Enable SLLI+SRLI to be fused to zero extension of halfword">;

def TuneZExtWFusion
: SubtargetFeature<"zextw-fusion", "HasZExtWFusion",
"true", "Enable SLLI+SRLI to be fused to zero extension of word">;

def TuneShiftedZExtWFusion
: SubtargetFeature<"shifted-zextw-fusion", "HasShiftedZExtWFusion",
"true", "Enable SLLI+SRLI to be fused when computing (shifted) zero extension of word">;

def TuneLDADDFusion
: SubtargetFeature<"ld-add-fusion", "HasLDADDFusion",
"true", "Enable LD+ADD macrofusion.">;
Expand Down
82 changes: 67 additions & 15 deletions llvm/lib/Target/RISCV/RISCVMacroFusion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,27 +58,74 @@ static bool isLDADD(const MachineInstr *FirstMI, const MachineInstr &SecondMI) {
return checkRegisters(FirstMI->getOperand(0).getReg(), SecondMI);
}

// Fuse these patterns:
//
// slli rd, rs1, 32
// srli rd, rd, x
// where 0 <= x <= 32
//
// and
//
// Fuse zero extension of halfword:
// slli rd, rs1, 48
// srli rd, rd, 48
static bool isZExtH(const MachineInstr *FirstMI, const MachineInstr &SecondMI) {
if (SecondMI.getOpcode() != RISCV::SRLI)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor, but a utility which matches a SRLI and a separate one which matches a SRLI and returns the immediate might reduce some code duplication through this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I just leave it just like this? This whole file will be removed in #72224.

return false;

if (!SecondMI.getOperand(2).isImm())
return false;

if (SecondMI.getOperand(2).getImm() != 48)
return false;

// Given SecondMI, when FirstMI is unspecified, we must return
// if SecondMI may be part of a fused pair at all.
if (!FirstMI)
return true;

if (FirstMI->getOpcode() != RISCV::SLLI)
return false;

if (FirstMI->getOperand(2).getImm() != 48)
return false;

return checkRegisters(FirstMI->getOperand(0).getReg(), SecondMI);
}

// Fuse zero extension of word:
// slli rd, rs1, 32
// srli rd, rd, 32
static bool isZExtW(const MachineInstr *FirstMI, const MachineInstr &SecondMI) {
if (SecondMI.getOpcode() != RISCV::SRLI)
return false;

if (!SecondMI.getOperand(2).isImm())
return false;

if (SecondMI.getOperand(2).getImm() != 32)
return false;

// Given SecondMI, when FirstMI is unspecified, we must return
// if SecondMI may be part of a fused pair at all.
if (!FirstMI)
return true;

if (FirstMI->getOpcode() != RISCV::SLLI)
return false;

if (FirstMI->getOperand(2).getImm() != 32)
return false;

return checkRegisters(FirstMI->getOperand(0).getReg(), SecondMI);
}

// Fuse shifted zero extension of word:
// slli rd, rs1, 32
// srli rd, rd, x
static bool isShiftedZExt(const MachineInstr *FirstMI,
const MachineInstr &SecondMI) {
// where 0 <= x < 32
static bool isShiftedZExtW(const MachineInstr *FirstMI,
const MachineInstr &SecondMI) {
if (SecondMI.getOpcode() != RISCV::SRLI)
return false;

if (!SecondMI.getOperand(2).isImm())
return false;

unsigned SRLIImm = SecondMI.getOperand(2).getImm();
bool IsShiftBy48 = SRLIImm == 48;
if (SRLIImm > 32 && !IsShiftBy48)
if (SRLIImm >= 32)
return false;

// Given SecondMI, when FirstMI is unspecified, we must return
Expand All @@ -89,8 +136,7 @@ static bool isShiftedZExt(const MachineInstr *FirstMI,
if (FirstMI->getOpcode() != RISCV::SLLI)
return false;

unsigned SLLIImm = FirstMI->getOperand(2).getImm();
if (IsShiftBy48 ? (SLLIImm != 48) : (SLLIImm != 32))
if (FirstMI->getOperand(2).getImm() != 32)
return false;

return checkRegisters(FirstMI->getOperand(0).getReg(), SecondMI);
Expand Down Expand Up @@ -144,7 +190,13 @@ static bool shouldScheduleAdjacent(const TargetInstrInfo &TII,
if (ST.hasAUIPCADDIFusion() && isAUIPCADDI(FirstMI, SecondMI))
return true;

if (ST.hasShiftedZExtFusion() && isShiftedZExt(FirstMI, SecondMI))
if (ST.hasZExtHFusion() && isZExtH(FirstMI, SecondMI))
return true;

if (ST.hasZExtWFusion() && isZExtW(FirstMI, SecondMI))
return true;

if (ST.hasShiftedZExtWFusion() && isShiftedZExtW(FirstMI, SecondMI))
return true;

if (ST.hasLDADDFusion() && isLDADD(FirstMI, SecondMI))
Expand Down
4 changes: 3 additions & 1 deletion llvm/lib/Target/RISCV/RISCVProcessors.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,9 @@ def VENTANA_VEYRON_V1 : RISCVProcessorModel<"veyron-v1",
[TuneVentanaVeyron,
TuneLUIADDIFusion,
TuneAUIPCADDIFusion,
TuneShiftedZExtFusion,
TuneZExtHFusion,
TuneZExtWFusion,
TuneShiftedZExtWFusion,
TuneLDADDFusion]>;

def XIANGSHAN_NANHU : RISCVProcessorModel<"xiangshan-nanhu",
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Target/RISCV/RISCVSubtarget.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,8 @@ class RISCVSubtarget : public RISCVGenSubtargetInfo {
}

bool hasMacroFusion() const {
return hasLUIADDIFusion() || hasAUIPCADDIFusion() ||
hasShiftedZExtFusion() || hasLDADDFusion();
return hasLUIADDIFusion() || hasAUIPCADDIFusion() || hasZExtHFusion() ||
hasZExtWFusion() || hasShiftedZExtWFusion() || hasLDADDFusion();
}

// Vector codegen related methods.
Expand Down
29 changes: 23 additions & 6 deletions ...CodeGen/RISCV/macro-fusions-veyron-v1.mir → llvm/test/CodeGen/RISCV/macro-fusions.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# REQUIRES: asserts
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename the slli_srli_... test to zext16... and zext32 and shifted_zext32_... to make it more consistent with other tests. I should have done this myself, but I've missed that :-<

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to add tests that exercise logic from checkRegisters, but we can leave it to a different PR

# RUN: llc -mtriple=riscv64-linux-gnu -mcpu=veyron-v1 -x=mir < %s \
# RUN: llc -mtriple=riscv64-linux-gnu -x=mir < %s \
# RUN: -debug-only=machine-scheduler -start-before=machine-scheduler 2>&1 \
# RUN: -mattr=+lui-addi-fusion,+auipc-addi-fusion,+shifted-zext-fusion,+ld-add-fusion \
# RUN: -mattr=+lui-addi-fusion,+auipc-addi-fusion,+zexth-fusion,+zextw-fusion,+shifted-zextw-fusion,+ld-add-fusion \
# RUN: | FileCheck %s

# CHECK: lui_addi:%bb.0
Expand Down Expand Up @@ -38,10 +38,10 @@ body: |
PseudoRET
...

# CHECK: slli_srli
# CHECK: slli_srli_shifted_zext
# CHECK: Macro fuse: {{.*}}SLLI - SRLI
---
name: slli_srli
name: shifted_zext
tracksRegLiveness: true
body: |
bb.0.entry:
Expand All @@ -55,10 +55,10 @@ body: |
PseudoRET
...

# CHECK: slli_srli_48
# CHECK: slli_srli_zexth
# CHECK: Macro fuse: {{.*}}SLLI - SRLI
---
name: slli_srli_48
name: zexth
tracksRegLiveness: true
body: |
bb.0.entry:
Expand All @@ -72,6 +72,23 @@ body: |
PseudoRET
...

# CHECK: slli_srli_zextw
# CHECK: Macro fuse: {{.*}}SLLI - SRLI
---
name: zextw
tracksRegLiveness: true
body: |
bb.0.entry:
liveins: $x10
%1:gpr = COPY $x10
%2:gpr = SLLI %1, 32
%3:gpr = XORI %1, 3
%4:gpr = SRLI %2, 32
$x10 = COPY %3
$x11 = COPY %4
PseudoRET
...

# CHECK: slli_srli_no_fusion_0
# CHECK-NOT: Macro fuse: {{.*}}SLLI - SRLI
---
Expand Down