This is an archive of the discontinued LLVM Phabricator instance.

[LoongArch 4/6] Add basic tablegen infra for LoongArch
ClosedPublic

Authored by SixWeining on Dec 16 2021, 12:51 AM.

Details

Summary

This patch introduces basic tablegen infra such as
LoongArch{InstrFormats,InstrInfo,RegisterInfo,CallingConv,}.td.

For now, only add instruction definitions for LoongArch basic integer
operations.
Our initial target is a working MC layer rather than codegen,
so appropriate SelectionDAG patterns will come later.

Depends on D115860

Diff Detail

Event Timeline

SixWeining created this revision.Dec 16 2021, 12:51 AM
SixWeining requested review of this revision.Dec 16 2021, 12:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2021, 12:51 AM

rename the patches number from n to 5

SixWeining retitled this revision from [LoongArch 4/n] Add basic tablegen infra for LoongArch to [LoongArch 4/5] Add basic tablegen infra for LoongArch.Dec 17 2021, 7:56 PM

Adding owners of recent new targets to help onboarding this new target.

I can't see anything wrong with this patch. It seems to have all the bits to start a target in the right places.

I'm assuming you have passed clang-format on all C++ files and headers.

As with the other patches, I'll leave this be until more people have had a look at it.

SixWeining retitled this revision from [LoongArch 4/5] Add basic tablegen infra for LoongArch to [LoongArch 4/6] Add basic tablegen infra for LoongArch.Dec 22 2021, 5:14 PM

rename getTheLA{32,64}Target to getTheLoongArch{32,64}Target

I can't see anything wrong with this patch. It seems to have all the bits to start a target in the right places.

I'm assuming you have passed clang-format on all C++ files and headers.

As with the other patches, I'll leave this be until more people have had a look at it.

Thanks @rengolin. Yes, we have passed clang-format on all C++ files and headers.

xen0n added a subscriber: xen0n.Jan 6 2022, 6:15 AM
xen0n added inline comments.
llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
44

As discussed in D115862, people should reach consensus on instruction format naming before continuing with coding.

llvm/lib/Target/LoongArch/LoongArchRegisterInfo.td
47

According to the LoongArch ELF psABI doc, we're pushing this code after the v0/v1 aliases are deprecated, so I think the aliases could simply be removed?

63

Is the "r21" alias needed, given it's identical to its canonical name?

103

Same here for fv0/fv1.

llvm/lib/Target/LoongArch/LoongArchSubtarget.cpp
32

Nit: why not something like generic-la64 to keep symmetry?

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
39

Typo: "determine"

45

Cannot figure this out even as Chinglish... Maybe you meant something like "Check for byte count not multiple of instruction word size"?

MaskRay added inline comments.Jan 6 2022, 11:00 AM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
45

And avoid unneeded parens: if (Count % 4 != 0)

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCAsmInfo.cpp
25

It's unnecessary to align =. Just remove alignment and // clang-format

Reply comments inline. I will update this revision soon.

llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
44

Right. But this patch is parent of D115862 but not child.

Do other people have any comments for the instruction format naming? @rengolin @MaskRay

llvm/lib/Target/LoongArch/LoongArchRegisterInfo.td
47

Yes, v0/v1 are deprecated. I will remove them. Thanks.

63

Right. This alias is unnecessary and I will remove it although other targets have similar usage, like the CSKY:

78   def R26 : CSKYReg<26, "r26", ["r26"]>, DwarfRegNum<[26]>;
79   def R27 : CSKYReg<27, "r27", ["r27"]>, DwarfRegNum<[27]>;
103

OK. I will remove them.

llvm/lib/Target/LoongArch/LoongArchSubtarget.cpp
32

la464 is the name of a real CPU core designed by Loongson while generic-la32 is a dummy CPU name.

Currently we only defined the la464 ProcessorModel in LoongArch.td:

94 def : ProcessorModel<"la464", NoSchedModel, [Feature64Bit]>;

Maybe we shoulde define another 2 ProcessMode: generic-la64 and generic-la32? And change

CPU = Is64Bit ? "la464" : "generic-la32";

to

CPU = Is64Bit ? "generic-la64" : "generic-la32";
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
39

Yes. Thanks.

45

Thanks. I will change that.

45

OK. Thanks.

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCAsmInfo.cpp
25

OK. I will change that. Thanks.

address comments from @xen0n and @MaskRay

In addition, add "s9" as the alias of register "r22".

add missing change to testcase

xen0n added inline comments.Jan 7 2022, 1:00 AM
llvm/lib/Target/LoongArch/LoongArchRegisterInfo.td
63

Hmm, I wonder if [""] or [] is correct; perhaps this "duplication" is necessary for proper functioning after all, I'm not quite sure. Maybe others more familiar with tblgen could clarify this.

SixWeining added inline comments.Jan 7 2022, 1:20 AM
llvm/lib/Target/LoongArch/LoongArchRegisterInfo.td
63

If we use [] , tblgen is not happy.
If we use [""], tblgen is happy but llvm-mc will crash. We must add this change:

let FallbackRegAltNameIndex = NoRegAltName in
def RegAliasName : RegAltNameIndex;
rengolin added inline comments.Jan 7 2022, 2:46 AM
llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
44

I don't actually have a strong opinion on this...

My personal view is that this is at such a low level that (almost) only people that work on the target will have to deal with, so what constitutes "easy to read and understand" depends on the target's own context.

For example abbreviations of target-specific terms may not make much sense to other people but they're very clear to that target engineers.

Having said that, I do appreciate @xen0n's point of having a consistent format. This is more than just "what feels right" and is more about "how easy it is for outside people understand the code (by reading an external doc, for ex.)" as well as "what scales better and is more maintainable".

The argument that this is a new target and we ought to do better is also a good one. But at the same time, if there are different back-ends to other tools and only this one is different, that'd add maintenance cost to the target's community.

And while I agree that setting a good precedent is worthwhile, there could be resistance from other compilers/emulators to refactor their code without clear benefits, so the difference may end up permanent.

SixWeining added inline comments.Jan 26 2022, 12:18 AM
llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
44

Thanks @rengolin @xen0n.

I think we'd better form a conclusion as soon as possible so that we can move on.

In the past days we tried to get more inputs from other people but no one leave their comments here. However, we discussed with other LoongArch compilers/emulators owners offline internally and just as @rengolin said they have no strong willing to modify the code by following the proposal from @xen0n especially in the case that GNU binutils has been upstreamed.

So we prefer to insist on current implementation in this patch. How do you think about?

rengolin added inline comments.Jan 26 2022, 2:16 AM
llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
44

Indeed, even if the LoongArch engineers were willing, I doubt the GNU maintainers will welcome formatting changes to gcc, binutils based on opinions from the LLVM community.

To be honest, I wouldn't change this now even if the GNU community was willing to change because I know how long it takes to change things in the GNU community when there isn't a strong consensus. (TBH, the same is true here).

So, even though I don't have a strong preference, I do have a strong opinion: let's not change this now, please.

xen0n added inline comments.Jan 26 2022, 7:17 AM
llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
44

Fine; it's true we (or rather I) have to first get the suggested scheme implemented in GNU first anyway, because it's even more important to maintain consistency across the different toolchains.

What I'd like to confirm is that the community still allows for the possibility for the current scheme to change, if the GNU side were to be updated. IMO setting this inconsistent mess into stone forever really is a shame, and it's entirely Loongson's fault to not discuss all of this upfront in a transparent way, eventually leading to this, but we don't have time machines and have to live on even though the whole conflict is avoidable. I'll continue to push for better and more consistent LoongArch documentation, implementation and teaching across the projects.

rengolin added inline comments.Jan 26 2022, 9:27 AM
llvm/lib/Target/LoongArch/LoongArchInstrFormats.td
44

You'll find that the LLVM community is much more amenable to refactoring than the GNU counterparts, so if you manage to convince them, we'll probably just follow suit.

I have tried coordinating language extensions, code generation and runtime behaviour with the GNU folks before (at a GNU cauldron and after that), but my efforts were met with disdain and nothing ever happened from there, so don't hold your breath.

rengolin accepted this revision.Feb 8 2022, 3:30 AM

AFAICS, all issues have been fixed and the last contentious was about changing naming on the GNU side, which is completely out of scope in this series.

I'm approving this because it looks good to me, but if anyone has any further comments, the 6th patch is not approved yet, so we have time.

This revision is now accepted and ready to land.Feb 8 2022, 3:30 AM
xen0n added a comment.Feb 8 2022, 4:52 AM

Aside from my pet peeves against instruction format naming (and I agree to first merging as-is), only one minor comment and we're good to go!

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h
37

Sorry for noticing this late, but don't we have simm21 for BEQZ/BNEZ and simm26 for B/BL?

Aside from my pet peeves against instruction format naming (and I agree to first merging as-is), only one minor comment and we're good to go!

Thanks for your agreement. I have replied the comment inline.

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h
37

Thanks. This patch is just to add basic tablegen infra, so not all kinds of operand type are added here. Branch and function call would be implemented in future and related operand types would be added then.

xen0n added inline comments.Feb 8 2022, 6:25 AM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h
37

Hmm, looking at surrounding code and usages of the other defines here, it seems all "lsl2" operands are not represented here, but correctly referenced in the td files; is that inconsistency intentional? Because branch instructions are clearly present in the tables.

xen0n added inline comments.Feb 8 2022, 7:37 AM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h
37

(silly English mistake in previous comment: "none of the lsl2 operands are represented here")

SixWeining added inline comments.Feb 8 2022, 6:36 PM
llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.h
37

The OperandType here is to be used to implement instruction verification in future. Let me remove them and add them back when necessary. Thanks.

SixWeining updated this revision to Diff 407033.EditedFeb 8 2022, 7:46 PM

Address @xen0n's comments by removing currently unused OperandType.

xen0n accepted this revision.Feb 8 2022, 11:03 PM

Seems good enough! I'll trust the other reviewers and maintainers here and just accept this.

(Thanks for fixing that nit, btw!)

This revision was landed with ongoing or failed builds.Feb 10 2022, 2:24 AM
This revision was automatically updated to reflect the committed changes.