[PATCH] D137517: [TargetParser] Generate the defs for RISCV CPUs using llvm-tblgen.

[Wang Pengcheng via Phabricator via llvm-commits]

pcwang-thead added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:568
 
-def : ProcessorModel<"generic-rv32", NoSchedModel, [Feature32Bit]>;
-def : ProcessorModel<"generic-rv64", NoSchedModel, [Feature64Bit]>;
+class RISCVProcessorModelPROC<string n,
+                              SchedMachineModel m,
----------------
RISCVProcessorModelPROC->RISCVProcessorModel?
RISCVProcessorModelTUNE_PROC->RISCVTuneProcessorModel?
I think it is a little weird that we mixed naming styles here.




================
Comment at: llvm/lib/Target/RISCV/RISCV.td:576
+
+class RISCVProcessorModelTUNE_PROC<string n, SchedMachineModel m, list<SubtargetFeature> f,
+                      list<SubtargetFeature> tunef = []> : ProcessorModel<n,m,f,tunef>;
----------------
As for ProcessorModels for tuning, `list<SubtargetFeature> f` is always empty in both upstream and our downstream, and it is unlikely that we will specify target features for tune models. So it can be a default argument with value `[]` and swap the position of `f` and `tunef`(we are more likely to specify tune features). It becomes:
```
class RISCVTuneProcessorModel<string n, SchedMachineModel m, list<SubtargetFeature> tunef = [],
    list<SubtargetFeature> f = []> : ProcessorModel<n,m,f,tunef>;
```
@craig.topper Any thoughts on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137517