[PATCH][Mips] ELF header flag support for assemblers

Rafael EspĂ­ndola rafael.espindola at gmail.com
Sun Nov 17 17:07:48 PST 2013


The direction in general looks good, but

* Please make the first patch a refactoring only moving
MipsTargetStreamer to its own file. That should make the following
patches easier to read. A patch doing just that LGTM.

* Try to keep tests in sync with the code. For example, a patch adding
parseDirectiveMipsSTO should include a .s test testing that. BTW, if
the diritive name is just .sto, the method name can be just
parseDirectiveSTO since.


> There are holes that need to be filled. Currently STO symbol handling is
> only on the integrated assembler side and needs to be done on the llvm-mc
> side as well. The MipsTargetSteamer objects are not set up for dyn_cast.

What do you want the dyn_cast for? The design of the stream interface
is to avoid having to take different actions for object and assembly
streamers.

> I
> don't know yet how far up it isn't ready, but we will want it fixed soon.
> There are still some integrated assembler direct object tests and they need
> to go once we have a unified handling of the ELF output.
>
> Are these patches good to commit?

A refactoring patch is OK as noted above. Please then post the patch
with test cases rebased on top.

> Jack

Cheers,
Rafael



More information about the llvm-commits mailing list