[PATCH] D123988: [LoongArch] Add basic floating-point instructions definition

[Lu Weining via Phabricator via llvm-commits]

SixWeining added a comment.

Reply to @rengolin comments inline. Thanks.



================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrFormatsF.td:1
+//==- LoongArchInstrFormatsF.td - LoongArch FP Instr Formats -*- tablegen -*-=//
+//
----------------
rengolin wrote:
> nit: `...InstrFormatF` is a bit cryptic. Perhaps `...FloatInstrFormat`?
Good suggestion. Thanks. I will change it.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrInfoD.td:1
+//=-- LoongArchInstrInfoD.td - Double-Precision Float instr -*- tablegen -*-==//
+//
----------------
rengolin wrote:
> Same nit on file naming.
Good suggestion. But I cannot figure out a good name. `...DoubleFloatInstrInfo.td` or `...DoubleInstrInfo.td`? Seems both are too long...


================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrInfoF.td:22
+
+class FP_ALU_3R<bits<17> op, string opstr, RegisterClass rc>
+    : FPFmt3R<op, (outs rc:$fd), (ins rc:$fj, rc:$fk), opstr, "$fd, $fj, $fk">;
----------------
rengolin wrote:
> These definitions are also used in the double file, but not included there and probably only works because you include this one first on the main TD file.
> 
> I'd move all common logic to `...InstrFormat` and include that on both `...InstrInfo` files.
Good suggestion. But I have a different point of view. 

In CPP file, it's recommended to include what it uses. Maybe it is because a CPP file is a compilation unit which could be compiled independently.

But for LLVM target's TD files, not all of them are independent compilation units. Tablegen only processes the `{Target}.td` that include all other TD files  `(..RegisterInfo.td, ..InstrFormats.td, ...InstrInfo.td)` directly or indirectly.

If we define TD file like CPP file, we may have to include `llvm/Target/Target.td` and `LoongArchRegisterInfo.td` in any `..InstrInfo.td` files. 

Anyway, the current files layout is really not very good. How about this improvement:

LoongArchInstrInfo.td
```
...
include "LoongArchFloatInstrFormats.td" // common logic for both F/D
...
include "LoongArchSingleFloatInstrInfo"
include "LoongArchDoubleFloatInstrInfo"
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123988