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

[Daniel Sanders]

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

> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Rafael Espíndola
> Sent: 26 January 2014 07:08
> To: Jack Carter
> Cc: llvm-commits
> Subject: Re: [llvm] r200051 - [Mips] TargetStreamer ELF flag Support for
> default and commandline options.
> 
> On 25 January 2014 15:45, Jack Carter <Jack.Carter at imgtec.com> wrote:
> > I will revert this.
> >
> > That said, I really need to set the default and commanline flags. I want to
> do this for AsmParser as well as AsmPrinter.
> >
> > Is it just the name of the method that is bothering you?
> 
> Have you actually read the email? Have you paid any attention to all the
> discussions over email and in person about what is the design of the
> MCStreamer interface? You just added a method that doesn't correspond to
> anything in a .s file. How can you possibly think that the issue was just the
> method name!?!?
> 
> I have finished implementing the bits of the object emission that were being
> hacked before (r200138). It is pretty clear you don't have a solid
> understanding of this part of the code, so please refrain from any commits
> before reviews in MC code from now on.
> 
> >Or is it the problem of the code generator not issuing the default and
> commandline directives directly out to the output file and have AsmParser
> never deal with commandline or default values?
> >
> > Is what you are saying is that I don't use a TargetStreamer interface for
> either? If I take out the MipsTargetSteamer part will you be happy? I don't
> think so because I cannot use hasRawTextSupport to discriminate between
> ELF and ASCII in MipsAsmPrinter.
> >
> > The Hack variation is not correct either. How do I set the EFLAGS from
> default and commandline flags at startup time?
> >
> > All the rest can be handled by the directives.
> >
> > Mips is not arm nor is it x86. It uses, possibly overuses the ELF header flags
> extensively for linking whereas other target use other means. Default values
> affect eflags, commandline options can override the default and or
> add/negate eflags, inline directives can add/negate eflags.
> >
> > I need a workable solution, not a hack. I request you help in resolving this
> problem.
> >
> > Sincerely,
> >
> > Jack
> > ________________________________________
> > From: Rafael Espíndola [rafael.espindola at gmail.com]
> > Sent: Saturday, January 25, 2014 6:58 AM
> > To: Jack Carter
> > Cc: llvm-commits
> > Subject: Re: [llvm] r200051 - [Mips] TargetStreamer ELF flag Support for
> default and commandline options.
> >
> >> +  getTargetStreamer().emitMipsELFFlags(EFlags);
> >> +}
> >
> > This is incorrect. If there is no assembly directive that takes and
> > sets eflags, there should not be any emitMipsELFFlags. At first look
> > this is not better than the hack version that was there before.
> >
> >
> >> +void MipsTargetAsmStreamer::emitMipsELFFlags(unsigned Flags) {
> >> +return; }
> >
> > At a second look this is *far* worse. You once again has put the mips
> > backend in a position where it has a different code path for text and
> > object files. Please revert this.
> >
> > If there are common flags that are set by default when reading a .s
> > file that should be implemented in the target streamer creation. It
> > should *not* be part of the streamer interface.
> >> +void MipsAsmPrinter::processInitialEFlags() {
> >> +  // Not having this check would work too, but would have us chew
> >> +through
> >> +  // code that it doesn't use for RawText.
> >> +  if (OutStreamer.hasRawTextSupport())
> >> +    return;
> >
> > Adding calls to hasRawTextSupport is not acceptable.
> >
> > Cheers,
> > Rafael
> >
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits