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

[WÁNG Xuěruì via Phabricator via llvm-commits]

xen0n 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:
> > 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?
> > 
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115861