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

Wed Apr 20 02:18:46 PDT 2022

SixWeining added a comment.

Thanks for your comments. @xry111

================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrInfoD.td:1
+//=-- LoongArchInstrInfoD.td - Double-Precision Float instr -*- tablegen -*-==//
+//
----------------
xry111 wrote:
> SixWeining wrote:
> > 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...
> I don't think a long filename will be a problem (it's not MS-DOS 8.3 era after all :).  Note that RISC-V uses "F" and "D" not for abbreviation, but because their ISA extension for 32-bit and 64-bit floating-point are simply named "F" and "D".
> 
> I'd suggest to use `LoongArchFloat32InstrInfo.td` and `LoongArchFloat64InstrInfo.td`.
> 
> BTW, can we omit `LoongArch` prefix for the `td` files?  For example, AMDGPU does not prefix every `td` files with `AMDGPU`.
> I don't think a long filename will be a problem (it's not MS-DOS 8.3 era after all :).  Note that RISC-V uses "F" and "D" not for abbreviation, but because their ISA extension for 32-bit and 64-bit floating-point are simply named "F" and "D".
> 
> I'd suggest to use `LoongArchFloat32InstrInfo.td` and `LoongArchFloat64InstrInfo.td`.
It looks good! Thanks
> 
> BTW, can we omit `LoongArch` prefix for the `td` files?  For example, AMDGPU does not prefix every `td` files with `AMDGPU`.
Good suggestion. Maybe we can rename them in a seperate patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123988