Page MenuHomePhabricator

[LoongArch] Add basic floating-point instructions definition
ClosedPublic

Authored by SixWeining on Apr 19 2022, 1:09 AM.

Details

Summary

These instructions are added by following the `LoongArch Reference
Manual Volume 1: Basic Architecture Version 1.00`.

Diff Detail

Event Timeline

SixWeining created this revision.Apr 19 2022, 1:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 1:09 AM
SixWeining requested review of this revision.Apr 19 2022, 1:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 1:09 AM

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"
xry111 added inline comments.Apr 20 2022, 2:11 AM
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)

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.

Address comments from @regolin and @xry111 about file naming and placement.

0. move some common logic to LoongArchFloatInstrFormats.td

  1. include LoongArchFloatInstrFormats.td in LoongArchInstrInfo.td but not LoongArchInstrInfoF.td
  2. LoongArchInstrInfoF.td -> LoongArchFloat32InstrInfo.td
  3. LoongArchInstrInfoD.td -> LoongArchFloat64InstrInfo.td
rengolin accepted this revision.Apr 20 2022, 10:24 AM

LGTM, thanks!

This revision is now accepted and ready to land.Apr 20 2022, 10:24 AM
xen0n added inline comments.Apr 20 2022, 8:44 PM
llvm/test/MC/LoongArch/Basic/Float/d-invalid.s
3

movgr2fr.d is not part of the D extension?

xen0n added a comment.Apr 20 2022, 8:45 PM

Welp I'm 1 hour late, quick hands... we could use some cleanups later though.

Welp I'm 1 hour late, quick hands... we could use some cleanups later though.

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.

xen0n added a comment.Apr 20 2022, 9:49 PM

Welp I'm 1 hour late, quick hands... we could use some cleanups later though.

Sorry, I should wait for a while.

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).

MaskRay added a comment.EditedApr 20 2022, 10:15 PM

Welp I'm 1 hour late, quick hands... we could use some cleanups later though.

Sorry, I should wait for a while.

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.

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.

Welp I'm 1 hour late, quick hands... we could use some cleanups later though.

Sorry, I should wait for a while.

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.

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.

Thanks for your reminding. It's OK to me. I will wait a while in future commits.

xry111 added inline comments.Apr 20 2022, 11:48 PM
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.

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."

Apologies, my approval had that connotation but I forgot to write the exact wording.