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.

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] Teach RISCVMakeCompressible handle Zca/Zcf/Zce/Zcd. #81844

Merged
merged 3 commits into from
Feb 22, 2024

Conversation

yetingk
Copy link
Contributor

@yetingk yetingk commented Feb 15, 2024

Make targets which don't have C but have Zca/Zcf/Zce/Zcd benefit from this pass.

Make targets which don't have C but have Zca/Zcf/Zce/Zcd benefit from this pass.
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 15, 2024

@llvm/pr-subscribers-backend-risc-v

Author: Yeting Kuo (yetingk)

Changes

Make targets which don't have C but have Zca/Zcf/Zce/Zcd benefit from this pass.


Full diff: https://github.com/llvm/llvm-project/pull/81844.diff

3 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp (+23-8)
  • (modified) llvm/lib/Target/RISCV/RISCVSubtarget.h (+4)
  • (modified) llvm/test/CodeGen/RISCV/make-compressible.mir (+4)
diff --git a/llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp b/llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp
index ff21fe1d406463..6759be63d46bab 100644
--- a/llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp
+++ b/llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp
@@ -143,19 +143,35 @@ static bool isCompressedReg(Register Reg) {
 // Return true if MI is a load for which there exists a compressed version.
 static bool isCompressibleLoad(const MachineInstr &MI) {
   const RISCVSubtarget &STI = MI.getMF()->getSubtarget<RISCVSubtarget>();
-  const unsigned Opcode = MI.getOpcode();
 
-  return Opcode == RISCV::LW || (!STI.is64Bit() && Opcode == RISCV::FLW) ||
-         Opcode == RISCV::LD || Opcode == RISCV::FLD;
+  switch (MI.getOpcode()) {
+  default:
+    return false;
+  case RISCV::LW:
+  case RISCV::LD:
+    return STI.hasStdExtCOrZca();
+  case RISCV::FLW:
+    return !STI.is64Bit() && STI.hasStdExtCOrZcfOrZce();
+  case RISCV::FLD:
+    return STI.hasStdExtCOrZcd();
+  }
 }
 
 // Return true if MI is a store for which there exists a compressed version.
 static bool isCompressibleStore(const MachineInstr &MI) {
   const RISCVSubtarget &STI = MI.getMF()->getSubtarget<RISCVSubtarget>();
-  const unsigned Opcode = MI.getOpcode();
 
-  return Opcode == RISCV::SW || (!STI.is64Bit() && Opcode == RISCV::FSW) ||
-         Opcode == RISCV::SD || Opcode == RISCV::FSD;
+  switch (MI.getOpcode()) {
+  default:
+    return false;
+  case RISCV::SW:
+  case RISCV::SD:
+    return STI.hasStdExtCOrZca();
+  case RISCV::FSW:
+    return !STI.is64Bit() && STI.hasStdExtCOrZcfOrZce();
+  case RISCV::FSD:
+    return STI.hasStdExtCOrZcd();
+  }
 }
 
 // Find a single register and/or large offset which, if compressible, would
@@ -324,8 +340,7 @@ bool RISCVMakeCompressibleOpt::runOnMachineFunction(MachineFunction &Fn) {
   const RISCVInstrInfo &TII = *STI.getInstrInfo();
 
   // This optimization only makes sense if compressed instructions are emitted.
-  // FIXME: Support Zca, Zcf, Zcd granularity.
-  if (!STI.hasStdExtC())
+  if (!STI.hasStdExtZca() && !STI.hasStdExtCOrZcfOrZce() && !STI.hasStdExtZcd())
     return false;
 
   for (MachineBasicBlock &MBB : Fn) {
diff --git a/llvm/lib/Target/RISCV/RISCVSubtarget.h b/llvm/lib/Target/RISCV/RISCVSubtarget.h
index 8c55efa69a6a5f..d23b0c684fdc23 100644
--- a/llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ b/llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -143,6 +143,10 @@ class RISCVSubtarget : public RISCVGenSubtargetInfo {
 #include "RISCVGenSubtargetInfo.inc"
 
   bool hasStdExtCOrZca() const { return HasStdExtC || HasStdExtZca; }
+  bool hasStdExtCOrZcd() const { return HasStdExtC || HasStdExtZcd; }
+  bool hasStdExtCOrZcfOrZce() const {
+    return HasStdExtC || HasStdExtZcf || HasStdExtZce;
+  }
   bool hasStdExtZvl() const { return ZvlLen != 0; }
   bool hasStdExtFOrZfinx() const { return HasStdExtF || HasStdExtZfinx; }
   bool hasStdExtDOrZdinx() const { return HasStdExtD || HasStdExtZdinx; }
diff --git a/llvm/test/CodeGen/RISCV/make-compressible.mir b/llvm/test/CodeGen/RISCV/make-compressible.mir
index 2105a13bc8c7b7..d718a21fa5d9e8 100644
--- a/llvm/test/CodeGen/RISCV/make-compressible.mir
+++ b/llvm/test/CodeGen/RISCV/make-compressible.mir
@@ -3,6 +3,10 @@
 # RUN:   -run-pass=riscv-make-compressible | FileCheck --check-prefix=RV32 %s
 # RUN: llc -o - %s -mtriple=riscv64 -mattr=+c,+f,+d -simplify-mir \
 # RUN:   -run-pass=riscv-make-compressible | FileCheck --check-prefix=RV64 %s
+# RUN: llc -o - %s -mtriple=riscv32 -mattr=+zca,+zcf,+zcd -simplify-mir \
+# RUN:   -run-pass=riscv-make-compressible | FileCheck --check-prefix=RV32 %s
+# RUN: llc -o - %s -mtriple=riscv64 -mattr=+zca,+zcd -simplify-mir \
+# RUN:   -run-pass=riscv-make-compressible | FileCheck --check-prefix=RV64 %s
 --- |
 
   define void @store_common_value(ptr %a, ptr %b, ptr %c) #0 {

@asb
Copy link
Contributor

asb commented Feb 15, 2024

Thanks - I've left a couple of comments inline. The logic would be simplified if we moved to C implying the appropriate Zc* extensions as you wouldn't need to check for C || Zc*. But that's a separate topic and we held back due to concern about interacting with older assemblers. It might merit a TODO note though.

Are you planning to follow-up with a patch to teach isCompressibleLoad and isCompressibleStore about half/byte loads+stores where Zcb is available?

@@ -143,6 +143,10 @@ class RISCVSubtarget : public RISCVGenSubtargetInfo {
#include "RISCVGenSubtargetInfo.inc"

bool hasStdExtCOrZca() const { return HasStdExtC || HasStdExtZca; }
bool hasStdExtCOrZcd() const { return HasStdExtC || HasStdExtZcd; }
bool hasStdExtCOrZcfOrZce() const {
Copy link
Contributor

@asb asb Feb 15, 2024

Choose a reason for hiding this comment

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

It would be better to define hasStdExtCOrZcf I think. In any case where the Zc* extensions are used, Zcf will always be defined if flw/fsw are supported (there's logic in RISCVISAInfo to add zcf if zce is specified alongside f).

Copy link
Collaborator

Choose a reason for hiding this comment

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

We do have a HasStdExtCOrZcfOrZce as a tablegen predicate already. There was a reason I included in Zce in there, but I don't remember exactly why.

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 think the reason is -mattr=+f,+zce does not set HasStdExtZcf.

# RUN: llc -o - %s -mtriple=riscv32 -mattr=+zca,+zcf,+zcd -simplify-mir \
# RUN: -run-pass=riscv-make-compressible | FileCheck --check-prefix=RV32 %s
# RUN: llc -o - %s -mtriple=riscv64 -mattr=+zca,+zcd -simplify-mir \
# RUN: -run-pass=riscv-make-compressible | FileCheck --check-prefix=RV64 %s
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add tests for zca without zcf/zcd, which would demonstrate the new logic you added works correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@yetingk
Copy link
Contributor Author

yetingk commented Feb 16, 2024

Are you planning to follow-up with a patch to teach isCompressibleLoad and isCompressibleStore about half/byte loads+stores where Zcb is available?

Sure. I am willing to do it.

@yetingk
Copy link
Contributor Author

yetingk commented Feb 22, 2024

Ping.

@@ -324,8 +340,7 @@ bool RISCVMakeCompressibleOpt::runOnMachineFunction(MachineFunction &Fn) {
const RISCVInstrInfo &TII = *STI.getInstrInfo();

// This optimization only makes sense if compressed instructions are emitted.
// FIXME: Support Zca, Zcf, Zcd granularity.
if (!STI.hasStdExtC())
if (!STI.hasStdExtZca() && !STI.hasStdExtCOrZcfOrZce() && !STI.hasStdExtZcd())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be enough to check STI.hasStdExtCOrZca(); here I think?

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 think you are right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

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

LGTM

@yetingk yetingk merged commit 7e97ae3 into llvm:main Feb 22, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants