[llvm] r200051 - [Mips] TargetStreamer ELF flag Support for default and commandline options.

[Daniel Sanders]

> -----Original Message-----
> From: Rafael Espíndola [mailto:rafael.espindola at gmail.com]
> Sent: 27 January 2014 16:23
> To: Daniel Sanders
> Cc: Jack Carter; llvm-commits
> Subject: Re: [llvm] r200051 - [Mips] TargetStreamer ELF flag Support for
> default and commandline options.
> 
> On 27 January 2014 08:23, Daniel Sanders <Daniel.Sanders at imgtec.com>
> wrote:
> > Hi Rafael,
> >
> > To be fair to Jack, this commit was reviewed internally by myself, Matheus
> Almeida, and Vladimir Medic before commit. I mention this because I don't
> think it's fair that Jack takes all of the criticism while we stand by and watch.
> 
> While it is up to mips what internal procedures you guys use, the criticism
> about lack of understanding of the general design of MC stands. I would
> suggest that simply doing reviews in the open would be the most
> productive.
> 
> > When I reviewed this patch, I considered it a step forwards on the basis
> that ELF output by the assembler would have the correct eflags without the
> need for the hack directive.
> 
> To restate my original email, the patch basically renamed
> emitMipsHackELFFlags with emitMipsELFFlags. The reason for the
> "MipsHack" was to clearly state that it was a hack. Created as a short term
> solution to get a working solution that was consistent with the MC design of
> one method per assembly directive. Just removing the marker would be
> bad, but the patch went much further: it created a completely separate and
> duplicated code path for the deciding which flags to use when going from a
> .s -> to a .o.
>
> The objective (even more important than the design by which it is
> achieved) of MC is that generated object files have a corresponding
> assembly file. That avoids us getting to the point where the .s we produce
> are no more than a not too reliable debug helper.

Just to clarify part of that objective. Should the assembly file be completely self-contained, or are command line arguments permitted to carry the initial state? The reason I ask is that I'm not 100% certain that the former is currently possible in all cases. For example, I'm fairly certain there is no directive equivalent to -mmsa in gas, although in this particular case we should definitely add one if it doesn’t already exist.

> You must agree that with
> r200051 it was truly trivial for MipsAsmParser::processInitialEFlags to get out
> of sync with emitELFHeaderFlagsCG in codegen. That would let the Mips
> backend produce a .o that *no* assembly file would assemble to.

I agree. The duplication of that code is one of the things I failed to spot.

> > I was then expecting follow-on commits to add the directives you have
> added. This would have brought us to the same end point but via a slightly
> different route.
> 
> While doing one step back, two steps forward is sometimes the best
> strategy, there was no indication that that was the direction things were
> going. It also requires quiet a bit of confidence on who is doing the changes,
> specially when, as noted above, the step back got us in a position where it
> was possible to extend the code path used by "llc -filetype=obj" and not fix
> the corresponding assembly path. Now, as for confidence:
> 
> * This patch (r200051) was wrong in pretty much the exact same way
> r195067 was wrong.
> * There was a code review in progress before for *adding* a use of
> .mips_hack_stocg instead of figuring out the correct directive (.set
> micromips, which I implemented in r199181)
> * I added the the hack directives back in October (r192035). I did that out of
> failure to get mips developers to understand what was needed. In it I
> stated:
> 
>     The hope is that the mips developers will replace the hack directives with
>     the same ones that gas uses and drop the -print-hack-directives flag.
> 
> and
> 
>     In summary, for any new work, two rules of the thumb are
>       * Don't use "llc -filetype=obj" in tests.
>       * Don't add calls to hasRawTextSupport.
> 
> As you can see, both were not acknowledged.
>
> So it was hard to have confidence that we would see the "two steps
> forward". Also, it was clearly unnecessary to take the step back.
> Follow the commits I did and you will see that the correct interface was
> implemented and used without ever going back to to having the asm
> codepath and the obj code path.
> 
> For comparison, Logan Chien and Saleem Abdulrasool did an excellent job at
> moving ARM to use the target streamer.

I can certainly understand your lack of confidence in us on the basis of that history.

In my opinion, some (but by no means all) of it has its roots in our lack of need for an assembler other than gas. This is likely why we've been rather slow to implement the real directives and have continued to use the hack directive. That situation is changing. We have a couple projects lined up that will need a non-GPL assembler. We are in the very early stages of deciding exactly what features we need in this assembler, when, and how to achieve some of the trickier bits. It's naturally tempting to say that it should support all existing MIPS assembly but some historical behaviour of the assembler like instruction reordering (.setreorder) and the reserved scratch register (.setat) makes this harder than it sounds.

> > That said, there are some things I missed in my review and really should
> have caught such as the addition of the hasRawTextSupport() call (I saw it
> but didn't attach sufficient importance to it), and the code duplication in
> MipsAsmParser::processInitialEFlags() and
> MipsAsmPrinter::processInitialEFlags(). I apologise to both you and Jack for
> that. I'll look out for these in future reviews.
> 
> Thanks, but I still think that for now code reviews in llvm-commits are
> necessary for anything touching MC before commits.
> Cheers,
> Rafael

That's fair enough. The intention wasn't to try and change your mind on that point. I just didn't think it was fair to let one person receive the full force of the criticism when the reviewers (including myself) also failed to notice it.