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][NFC] Move Zawrs/Zacas implementation to RISCVInstrInfoZa.td #76940

Merged
merged 1 commit into from Jan 8, 2024

Conversation

wangpc-pp
Copy link
Contributor

To keep the structure of TableGen files clear.

The definitions are simplified by the way.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 4, 2024

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

Author: Wang Pengcheng (wangpc-pp)

Changes

To keep the structure of TableGen files clear.

The definitions are simplified by the way.


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

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.td (+1-16)
  • (added) llvm/lib/Target/RISCV/RISCVInstrInfoZawrs.td (+26)
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.td b/llvm/lib/Target/RISCV/RISCVInstrInfo.td
index 35e8edf5d2fa72..066dd07a2ee7f0 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.td
@@ -729,22 +729,6 @@ def UNIMP : RVInstI<0b001, OPC_SYSTEM, (outs), (ins), "unimp", "">,
   let imm12 = 0b110000000000;
 }
 
-let Predicates = [HasStdExtZawrs] in {
-def WRS_NTO : RVInstI<0b000, OPC_SYSTEM, (outs), (ins), "wrs.nto", "">,
-              Sched<[]> {
-  let rs1 = 0;
-  let rd = 0;
-  let imm12 = 0b000000001101;
-}
-
-def WRS_STO : RVInstI<0b000, OPC_SYSTEM, (outs), (ins), "wrs.sto", "">,
-              Sched<[]> {
-  let rs1 = 0;
-  let rd = 0;
-  let imm12 = 0b000000011101;
-}
-} // Predicates = [HasStdExtZawrs]
-
 } // hasSideEffects = 1, mayLoad = 0, mayStore = 0
 
 def CSRRW : CSR_ir<0b001, "csrrw">;
@@ -2095,6 +2079,7 @@ include "RISCVInstrInfoM.td"
 
 // Atomic
 include "RISCVInstrInfoA.td"
+include "RISCVInstrInfoZawrs.td"
 
 // Scalar FP
 include "RISCVInstrInfoF.td"
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZawrs.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZawrs.td
new file mode 100644
index 00000000000000..fac95eec814a46
--- /dev/null
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZawrs.td
@@ -0,0 +1,26 @@
+//===-- RISCVInstrInfoZawrs.td --------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file describes the RISC-V instructions from the standard
+// Wait-on-Reservation-Set (Zawrs) extension document.
+//
+//===----------------------------------------------------------------------===//
+
+let hasSideEffects = 1, mayLoad = 0, mayStore = 0 in {
+class WRSInst<bits<12> funct12, string opcodestr>
+    : RVInstI<0b000, OPC_SYSTEM, (outs), (ins), opcodestr, ""> {
+  let rs1 = 0;
+  let rd = 0;
+  let imm12 = funct12;
+}
+}
+
+let Predicates = [HasStdExtZawrs] in {
+def WRS_NTO : WRSInst<0b000000001101, "wrs.nto">, Sched<[]>;
+def WRS_STO : WRSInst<0b000000011101, "wrs.sto">, Sched<[]>;
+} // Predicates = [HasStdExtZawrs]

Copy link
Contributor

@asb asb left a comment

Choose a reason for hiding this comment

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

LGTM. I think it's not necessarily the case that every extension should have it's own file, but Zawrs would better belong in RISCVInstrInfoA or RISCVInstrInfoZa.td I think if we felt it didn't deserve its own file.

@topperc
Copy link
Collaborator

topperc commented Jan 4, 2024

LGTM. I think it's not necessarily the case that every extension should have it's own file

I agree. Adding a file for every extension doesn't scale well if we keep adding extensions at the rate we are.

@wangpc-pp
Copy link
Contributor Author

LGTM. I think it's not necessarily the case that every extension should have it's own file

I agree. Adding a file for every extension doesn't scale well if we keep adding extensions at the rate we are.

What about adding it to RISCVInstrInfoZa.td? We will have Zabha (https://github.com/riscv/riscv-zabha) and may have Zalasr (https://github.com/mehnadnerd/riscv-zalasr) later.

And what is the policy? Should we put all (or these simple ones) Zi* extensions into RISCVInstrInfoZi.td for example? We already have RISCVInstrInfoZb.td and RISCVInstrInfoZc.td.

@asb
Copy link
Contributor

asb commented Jan 4, 2024

LGTM. I think it's not necessarily the case that every extension should have it's own file

I agree. Adding a file for every extension doesn't scale well if we keep adding extensions at the rate we are.

What about adding it to RISCVInstrInfoZa.td? We will have Zabha (https://github.com/riscv/riscv-zabha) and may have Zalasr (https://github.com/mehnadnerd/riscv-zalasr) later.

We could move Zacas there too.

And what is the policy? Should we put all (or these simple ones) Zi* extensions into RISCVInstrInfoZi.td for example? We already have RISCVInstrInfoZb.td and RISCVInstrInfoZc.td.

It would be nice if you could always find a Zfoo extension by opening RISCVInstrInfoZfoo.td for RISCVInstrInfoZf.td Sadly, the subset extensions like Zmmul or Zca mean you'd need to look in the single-letter RISCVInstrInfo td file too. So that has to be a special case.

@wangpc-pp wangpc-pp changed the title [RISCV][NFC] Move Zawrs implementation to RISCVInstrInfoZawrs.td [RISCV][NFC] Move Zawrs implementation to RISCVInstrInfoZa.td Jan 5, 2024
To keep the structure of TableGen files clear.

The definitions are simplified by the way.
@wangpc-pp wangpc-pp changed the title [RISCV][NFC] Move Zawrs implementation to RISCVInstrInfoZa.td [RISCV][NFC] Move Zawrs/Zacas implementation to RISCVInstrInfoZa.td Jan 5, 2024
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

@wangpc-pp wangpc-pp merged commit b58a97d into llvm:main Jan 8, 2024
4 checks passed
@wangpc-pp wangpc-pp deleted the main-riscv-zawrs branch January 8, 2024 03:05
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
…lvm#76940)

To keep the structure of TableGen files clear.

The definitions are simplified by the way.
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