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

[Rafael Espíndola]

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

> 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