These instructions are added by following the `LoongArch Reference
Manual Volume 1: Basic Architecture Version 1.00`.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks reasonable to me. Some nits on file naming and definition placement. Thanks!
llvm/lib/Target/LoongArch/LoongArchInstrFormatsF.td | ||
---|---|---|
1 ↗ | (On Diff #423551) | nit: ...InstrFormatF is a bit cryptic. Perhaps ...FloatInstrFormat? |
llvm/lib/Target/LoongArch/LoongArchInstrInfoD.td | ||
1 ↗ | (On Diff #423551) | Same nit on file naming. |
llvm/lib/Target/LoongArch/LoongArchInstrInfoF.td | ||
22 ↗ | (On Diff #423551) | 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. |
Reply to @rengolin comments inline. Thanks.
llvm/lib/Target/LoongArch/LoongArchInstrFormatsF.td | ||
---|---|---|
1 ↗ | (On Diff #423551) | Good suggestion. Thanks. I will change it. |
llvm/lib/Target/LoongArch/LoongArchInstrInfoD.td | ||
1 ↗ | (On Diff #423551) | Good suggestion. But I cannot figure out a good name. ...DoubleFloatInstrInfo.td or ...DoubleInstrInfo.td? Seems both are too long... |
llvm/lib/Target/LoongArch/LoongArchInstrInfoF.td | ||
22 ↗ | (On Diff #423551) | 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" |
llvm/lib/Target/LoongArch/LoongArchInstrInfoD.td | ||
---|---|---|
1 ↗ | (On Diff #423551) | 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. |
Thanks for your comments. @xry111
llvm/lib/Target/LoongArch/LoongArchInstrInfoD.td | ||
---|---|---|
1 ↗ | (On Diff #423551) |
It looks good! Thanks
Good suggestion. Maybe we can rename them in a seperate patch. |
Address comments from @regolin and @xry111 about file naming and placement.
0. move some common logic to LoongArchFloatInstrFormats.td
- include LoongArchFloatInstrFormats.td in LoongArchInstrInfo.td but not LoongArchInstrInfoF.td
- LoongArchInstrInfoF.td -> LoongArchFloat32InstrInfo.td
- LoongArchInstrInfoD.td -> LoongArchFloat64InstrInfo.td
llvm/test/MC/LoongArch/Basic/Float/d-invalid.s | ||
---|---|---|
3 | movgr2fr.d is not part of the D extension? |
Sorry, I should wait for a while.
llvm/test/MC/LoongArch/Basic/Float/d-invalid.s | ||
---|---|---|
3 | Yes, movgr2fr.d is part of the D extension. But this test checks it is not available on LA32 as the document says: MOVGR2FR.D writes the 64-bit value of general register rj into floating-point register fd. On LA32, general register is 32-bit but not 64-bit. So MOVGR2FR.D is not available. |
Not particularly your fault; I've been busy in $DAY_JOB so I'm not paying as close attention as maybe 1 month ago. The comment is more-or-less nitpicky though, no harm in actual functionality.
llvm/test/MC/LoongArch/Basic/Float/d-invalid.s | ||
---|---|---|
3 | Hmm, for me the message comes as a bit of surprise, because it seems I'm getting asked for integer ISA on a FP insn; I don't know what others (i.e. RISC-V) are doing in this case though (memory is failing me and I cannot devote much time to LLVM before mid-May due to $DAY_JOB). |
https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted says "If it is likely that others will want to review a recently-posted patch, especially if there might be objections, but no one else has done so yet, it is also polite to provide a qualified approval (e.g., “LGTM, but please wait for a couple of days in case others wish to review”). If approval is received very quickly, a patch author may also elect to wait before committing (and this is certainly considered polite for non-trivial patches). Especially given the global nature of our community, this waiting time should be at least 24 hours. Please also be mindful of weekends and major holidays."
In this case @xen0n is very active on various things of LoongArch and often has opinions on the implementation, so it could be nicer to wait a bit.
This is perhaps more relevant for new contributors.
llvm/test/MC/LoongArch/Basic/Float/d-invalid.s | ||
---|---|---|
3 | RV uses let Predicates = [HasStdExtD, IsRV64] for FMV_D_X, so I guess it will emit a general "unrecognized instruction" diagnostic. |
movgr2fr.d is not part of the D extension?