[PATCH] D98136: [RISCV][RFC] Initially support the K-extension instructions on the LLVM MC layer

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:202
+                        FeatureStdExtZknh]>;
+def HasStdExtZkn : Predicate<"Subtarget->hasStdExtZkn()">,
+                             AssemblerPredicate<(all_of FeatureStdExtZkn),
----------------
Is this Predicate needed? Zkn is just a convenience to enable multiple extensions. No instructions belong to it right?


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:211
+                        FeatureStdExtZksh]>;
+def HasStdExtZks : Predicate<"Subtarget->hasStdExtZks()">,
+                             AssemblerPredicate<(all_of FeatureStdExtZks),
----------------
Is this Predicate needed? Zks is just a convenience to enable multiple extensions. No instructions belong to it right?


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:218
+                       "'Zkt' (Data Independent Execution Latency)">;
+def HasStdExtZkt : Predicate<"Subtarget->hasStdExtZkt()">,
+                             AssemblerPredicate<(all_of FeatureStdExtZkt),
----------------
This Predicate doesn't appear to be used on any instructions. Just the scheduler model


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:228
+                        FeatureStdExtZkt]>;
+def HasStdExtZk : Predicate<"Subtarget->hasStdExtZk()">,
+                             AssemblerPredicate<(all_of FeatureStdExtZk),
----------------
Is this Predicate needed? Zk is just a convenience to enable multiple extensions. No instructions belong to it right?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZk.td:37
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+class RVKShift_ri<bits<5> imm11_7, bits<3> funct3, RISCVOpcode opcode,
+                  string opcodestr>
----------------
Is this class used?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZk.td:44
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+class RVKShiftW_ri<bits<7> imm11_5, bits<3> funct3, RISCVOpcode opcode,
+                   string opcodestr>
----------------
Is this class used?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZk.td:53
+    : RVInstI<funct3, OPC_OP_IMM, (outs GPR:$rd), (ins GPR:$rs1),
+                   opcodestr, "$rd, $rs1">, Sched<[]> {
+  let imm12 = imm12_in;
----------------
craig.topper wrote:
> line this up with funct3 on the previous line
I don't think we should put `Sched<[]>` inside the generic classes. We'll need to be able to add per instruction information in the future.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZk.td:71
+    bits<4> rnum;
+        let Inst{31-25} = funct7;
+    let Inst{24} = 1;
----------------
This line is over indented


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98136/new/

https://reviews.llvm.org/D98136