[PATCH] D115861: [LoongArch 4/6] Add basic tablegen infra for LoongArch

[Lu Weining via Phabricator via llvm-commits]

SixWeining added inline comments.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrFormats.td:43
+// <opcode | rj | rd>
+class Fmt2R<bits<22> op, dag outs, dag ins, string asmstr,
+            list<dag> pattern = []> : LAInst<outs, ins, asmstr, pattern> {
----------------
rengolin wrote:
> SixWeining wrote:
> > xen0n wrote:
> > > As discussed in D115862, people should reach consensus on instruction format naming before continuing with coding.
> > Right. But this patch is parent of D115862 but not child.
> > 
> > Do other people have any comments for the instruction format naming? @rengolin @MaskRay 
> 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.
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?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115861