[PATCH][AsmParser][MC] Allow asm parser to emit elf header flags
Jim Grosbach
grosbach at apple.com
Mon Aug 26 14:08:04 PDT 2013
Hi Vladimir,
Detailed comments inline below.
Generally speaking, please run new code through clang-format. There are lots of small formatting problems here and there that tool will help clean up without having to fuss about the details.
I don’t have much to comment on regarding the Mips specific side of things. You guys know that stuff better than I do.
> Index: test/MC/Mips/mips_directives.s
> ===================================================================
> --- test/MC/Mips/mips_directives.s (revision 188764)
> +++ test/MC/Mips/mips_directives.s (working copy)
> @@ -3,7 +3,10 @@
> # CHECK: .text
> # CHECK: $BB0_2:
> $BB0_2:
> - .ent directives_test
> + .ent directives_test
> + .abicalls
> + .option pic0
> + .set mips16
> .frame $sp,0,$ra
> .mask 0x00000000,0
> .fmask 0x00000000,0
> Index: include/llvm/MC/MCTargetAsmParser.h
> ===================================================================
> --- include/llvm/MC/MCTargetAsmParser.h (revision 188764)
> +++ include/llvm/MC/MCTargetAsmParser.h (working copy)
> @@ -174,6 +174,10 @@
>
> virtual void convertToMapAndConstraints(unsigned Kind,
> const SmallVectorImpl<MCParsedAsmOperand*> &Operands) = 0;
> +
> + /// setAssemblerDialect - Target specific cleanup such as Ehdr flag setting.
> + /// Analogous to MipsAsmPrinter::EmitEndOfAsmFile().
> + virtual void emitEndOfObjFile(MCStreamer &Out) { }
The comment doesn’t match the function.
Relatedly, please refer to http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments for the latest style preferences for these comments. I wouldn’t trust that the surrounding code is up-to-date.
That said, why is this function necessary at all? EmitEndOfAsmFile() is called even for object files.
> };
>
> } // End llvm namespace
> Index: lib/Target/Mips/AsmParser/MipsAsmParser.cpp
> ===================================================================
> --- lib/Target/Mips/AsmParser/MipsAsmParser.cpp (revision 188764)
> +++ lib/Target/Mips/AsmParser/MipsAsmParser.cpp (working copy)
> @@ -7,6 +7,8 @@
> //
> //===----------------------------------------------------------------------===//
>
> +#include "AsmParser/MipsAsmFlags.h"
> +#include "MCTargetDesc/MipsELFStreamer.h"
> #include "MCTargetDesc/MipsMCTargetDesc.h"
> #include "MipsRegisterInfo.h"
> #include "llvm/ADT/StringSwitch.h"
> @@ -63,6 +65,7 @@
> MCSubtargetInfo &STI;
> MCAsmParser &Parser;
> MipsAssemblerOptions Options;
> + MipsMCAsmFlags Flags;
> bool hasConsumedDollar;
>
> #define GET_ASSEMBLER_HEADER
> @@ -166,6 +169,8 @@
>
> bool parseDirectiveWord(unsigned Size, SMLoc L);
>
> + bool parseDirectiveOption();
> +
> MCSymbolRefExpr::VariantKind getVariantKind(StringRef Symbol);
>
> bool isMips64() const {
> @@ -198,11 +203,20 @@
>
> bool processInstruction(MCInst &Inst, SMLoc IDLoc,
> SmallVectorImpl<MCInst> &Instructions);
> + void emitEndOfObjFile(MCStreamer &Out);
> +
> + void setAbiTy();
> + void setArchTy();
> + void setAseTy();
setXXX() is the usual name for the setter of a getter/setter pair. These are a different sort of thing, and should be named differently. Comments about what they do would be good.
> +
> public:
> MipsAsmParser(MCSubtargetInfo &sti, MCAsmParser &parser)
> : MCTargetAsmParser(), STI(sti), Parser(parser), hasConsumedDollar(false) {
> // Initialize the set of available features.
> setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
> + setAbiTy();
> + setArchTy();
> + setAseTy();
Since these are just called once, and all in order, why have three separate functions at all?
> }
>
> MCAsmParser &getParser() const { return Parser; }
> @@ -446,6 +460,43 @@
> return MipsInsts[Opcode];
> }
>
> +void MipsAsmParser::setAbiTy() {
> + uint64_t FeatureBits = STI.getFeatureBits();
> + unsigned AbiType = MipsMCAsmFlags::MAF_ABI_DEFAULT;
> + if (FeatureBits & Mips::FeatureN32)
> + AbiType = MipsMCAsmFlags::MAF_ABI_N32;
> + else if (FeatureBits & Mips::FeatureO32)
> + AbiType = MipsMCAsmFlags::MAF_ABI_O32;
> + else if (FeatureBits & Mips::FeatureN64)
> + AbiType = MipsMCAsmFlags::MAF_ABI_N64;
> +
> + Flags.setAbi(AbiType);
> +}
> +void MipsAsmParser::setArchTy() {
> + uint64_t FeatureBits = STI.getFeatureBits();
> + unsigned ArchType = MipsMCAsmFlags::MAF_ARCH_DEFAULT;
> + if (FeatureBits & Mips::FeatureMips64r2)
> + ArchType = MipsMCAsmFlags::MAF_ARCH_64R2;
> + else if (FeatureBits & Mips::FeatureMips64)
> + ArchType = MipsMCAsmFlags::MAF_ARCH_64R1;
> + else if (FeatureBits & Mips::FeatureMips32r2)
> + ArchType = MipsMCAsmFlags::MAF_ARCH_32R2;
> + else if (FeatureBits & Mips::FeatureMips32)
> + ArchType = MipsMCAsmFlags::MAF_ARCH_32R1;
> +
> + Flags.setArch(ArchType);
> +
> +}
> +void MipsAsmParser::setAseTy() {
> + uint64_t FeatureBits = STI.getFeatureBits();
> + unsigned AseType = MipsMCAsmFlags::MAF_ASE_DEFAULT;
> + if (FeatureBits & Mips::FeatureMips16)
> + AseType = MipsMCAsmFlags::MAF_ASE_MIPS16;
> + else if (FeatureBits & Mips::FeatureMicroMips)
> + AseType = MipsMCAsmFlags::MAF_ASE_MICROMIPS;
> + Flags.setAse(AseType);
> +}
> +
> bool MipsAsmParser::processInstruction(MCInst &Inst, SMLoc IDLoc,
> SmallVectorImpl<MCInst> &Instructions) {
> const MCInstrDesc &MCID = getInstDesc(Inst.getOpcode());
> @@ -1807,14 +1858,30 @@
> } else if (Tok.getString() == "nomacro") {
> return parseSetNoMacroDirective();
> } else if (Tok.getString() == "nomips16") {
> - // Ignore this directive for now.
> - Parser.eatToEndOfStatement();
> + if ((STI.getFeatureBits() & Mips::FeatureMips16))
> + STI.ToggleFeature(Mips::FeatureMips16);
> + Flags.setAse(MipsMCAsmFlags::MAF_ASE_DEFAULT);
> + Parser.Lex();
> return false;
> - } else if (Tok.getString() == "nomicromips") {
> - // Ignore this directive for now.
> - Parser.eatToEndOfStatement();
> + } else if (Tok.getString() == "mips16") {
> + if (!(STI.getFeatureBits() & Mips::FeatureMips16))
> + STI.ToggleFeature(Mips::FeatureMips16);
> + Flags.setAse(MipsMCAsmFlags::MAF_ASE_MIPS16);
> + Parser.Lex();
> return false;
> - } else {
> + }else if (Tok.getString() == "nomicromips") {
> + if (STI.getFeatureBits() & Mips::FeatureMicroMips)
> + STI.ToggleFeature(Mips::FeatureMicroMips);
> + Flags.setAse(MipsMCAsmFlags::MAF_ASE_DEFAULT);
> + Parser.Lex();
> + return false;
> + } else if (Tok.getString() == "micromips") {
> + if (!(STI.getFeatureBits() & Mips::FeatureMicroMips))
> + STI.ToggleFeature(Mips::FeatureMicroMips);
> + Flags.setAse(MipsMCAsmFlags::MAF_ASE_MICROMIPS);
> + Parser.Lex();
> + return false;
> + }else {
> // It is just an identifier, look for an assignment.
> parseSetAssignment();
> return false;
> @@ -1848,6 +1915,36 @@
> return false;
> }
>
> +bool MipsAsmParser::parseDirectiveOption() {
> + // Get the option token
> + AsmToken Tok = Parser.getTok();
> + // At the moment only identifiers are supported
> + if (Tok.isNot(AsmToken::Identifier)) {
> + // Clear line
> + Parser.eatToEndOfStatement();
> + return Error(Tok.getLoc(), "unexpected token in directive");
> + }
> + StringRef Option = Tok.getIdentifier();
> + if (Option == "pic0") {
> + Flags.setRelocationModel(MipsMCAsmFlags::MAF_RM_STATIC);
> + Parser.Lex();
> + if (Parser.getTok().isNot(AsmToken::EndOfStatement))
> + return Error(Parser.getTok().getLoc(), "unexpected token in directive");
> + return false;
> + }
> +
> + if (Option == "pic2") {
> + Flags.setRelocationModel(MipsMCAsmFlags::MAF_RM_PIC);
> + Parser.Lex();
> + if (Parser.getTok().isNot(AsmToken::EndOfStatement))
> + return Error(Parser.getTok().getLoc(), "unexpected token in directive");
> + return false;
> + }
> + // Unknown option
> + Parser.eatToEndOfStatement();
> + return Warning(Parser.getTok().getLoc(), "unknown option in directive");
> +}
> +
> bool MipsAsmParser::ParseDirective(AsmToken DirectiveID) {
>
> StringRef IDVal = DirectiveID.getString();
> @@ -1897,9 +1994,29 @@
> return false;
> }
>
> + if (IDVal == ".abicalls") {
> + // TODO: Set the Reloc::PIC_
> + Flags.setRelocationModel(MipsMCAsmFlags::MAF_RM_PIC);
> + if (Parser.getTok().isNot(AsmToken::EndOfStatement))
> + return Error(Parser.getTok().getLoc(), "unexpected token in directive");
> + return false;
> + }
> + if (IDVal == ".option") {
> + return parseDirectiveOption();
> + }
> +
> return true;
> }
>
> +/// setAssemblerDialect -
> +/// Analogous to MipsAsmPrinter::EmitEndOfAsmFile().
Incorrect comment again.
> +void MipsAsmParser::emitEndOfObjFile(MCStreamer &OutStreamer)
> +{
> + if (MipsELFStreamer *MES = dyn_cast<MipsELFStreamer>(&OutStreamer))
> + MES->emitELFHeaderFlagsAsm(Flags);
> + MCTargetAsmParser::emitEndOfObjFile(OutStreamer);
> +}
This can definitely be done without this function being added. See how ARM handles the EHDR flags.
> +
> extern "C" void LLVMInitializeMipsAsmParser() {
> RegisterMCAsmParser<MipsAsmParser> X(TheMipsTarget);
> RegisterMCAsmParser<MipsAsmParser> Y(TheMipselTarget);
> Index: lib/Target/Mips/AsmParser/MipsAsmFlags.h
> ===================================================================
> --- lib/Target/Mips/AsmParser/MipsAsmFlags.h (revision 0)
> +++ lib/Target/Mips/AsmParser/MipsAsmFlags.h (revision 0)
> @@ -0,0 +1,93 @@
> +//=== MipsMCAsmFlags.h - MipsMCAsmFlags --------------------------------===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENCE.TXT for details.
> +//
> +//===-------------------------------------------------------------------===//
> +#ifndef MIPSMCASMFLAGS_H_
> +#define MIPSMCASMFLAGS_H_
> +
> +namespace llvm {
> +class MipsMCAsmFlags;
> +
> +class MipsMCAsmFlags {
> +public:
> + enum MAFAbiTy {
> + MAF_ABI_DEFAULT,
> + MAF_ABI_O32,
> + MAF_ABI_N32,
> + MAF_ABI_N64
> + } MAFAbi;
> +
> + // These act as bit flags because more that one can be
> + // active at the same time, sometimes ;-)
> + enum MAFRelocationModelTy {
> + MAF_RM_DEFAULT = 0,
> + MAF_RM_STATIC = 1,
> + MAF_RM_CPIC = 2,
> + MAF_RM_PIC = 4
> + } MAFRelocationModel;
> +
> + enum MAFArchTy {
> + MAF_ARCH_DEFAULT,
> + MAF_ARCH_32R1,
> + MAF_ARCH_32R2,
> + MAF_ARCH_64R1,
> + MAF_ARCH_64R2
> + } MAFArch;
> +
> + enum MAFAseTy {
> + MAF_ASE_DEFAULT,
> + MAF_ASE_MIPS16,
> + MAF_ASE_MICROMIPS
> + } MAFAse;
> +
Shouldn’t these be part of the MIPS ELF object file information if they’re not already? They’re not really internal to the MIPS AsmParser, right?
> +public:
> + MipsMCAsmFlags()
> + : abi(MAF_ABI_DEFAULT),
> + model(MAF_RM_DEFAULT),
> + arch(MAF_ARCH_DEFAULT),
> + ase(MAF_ASE_DEFAULT) {}
> +
> + ~MipsMCAsmFlags() {}
> +
> + // This is simplistic. Mips16 may need on off switches.
> + void setAbi(unsigned Abi) {(abi = Abi);}
> + void setRelocationModel(unsigned RM) {(model |= RM);}
> + void setArch(unsigned Arch) {(arch = Arch);}
> + void setAse(unsigned Ase) {(ase = Ase);}
> +
> + bool isAbiO32() const {return (abi == MAF_ABI_O32);}
> + bool isAbiN32() const {return (abi == MAF_ABI_N32);}
> + bool isAbiN64() const {return (abi == MAF_ABI_N64);}
> + bool isAbiDefault() const {return (abi == MAF_ABI_DEFAULT);}
> +
> + bool isModelCpic() const {return (model & MAF_RM_CPIC) == MAF_RM_CPIC;}
> + bool isModelPic() const {return (model & MAF_RM_PIC) == MAF_RM_PIC;}
> + bool isModelStatic() const {return (model & MAF_RM_STATIC) == MAF_RM_STATIC;}
> + bool isModelDefault() const {
> + return (model & MAF_RM_DEFAULT) == MAF_RM_DEFAULT;
> + }
> +
> + bool isArch32r1() const {return (arch == MAF_ARCH_32R1);}
> + bool isArch32r2() const {return (arch == MAF_ARCH_32R2);}
> + bool isArch64r1() const {return (arch == MAF_ARCH_64R1);}
> + bool isArch64r2() const {return (arch == MAF_ARCH_64R2);}
> + bool isArchDefault() const {return (arch == MAF_ARCH_DEFAULT);}
> +
> + bool isAseMips16() const {return (ase == MAF_ASE_MIPS16);}
> + bool isAseMicroMips() const {return (ase == MAF_ASE_MICROMIPS);}
> + bool isAseDefault() const {return (ase == MAF_ASE_DEFAULT);}
> +
> +private:
> + unsigned abi; // o32, n32, n64, etc.
> + unsigned model; // pic, cpic, etc.
> + unsigned arch; // 32, 32r2, 64,etc.
> + unsigned ase; // 16, MicroMips, etc.
Class member variables shouldn’t be all-lower-case. See http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> +};
> +
> +}
> +
> +#endif /* MIPSMCASMFLAGS_H_ */
> Index: lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp
> ===================================================================
> --- lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp (revision 188764)
> +++ lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp (working copy)
> @@ -6,6 +6,7 @@
> // License. See LICENSE.TXT for details.
> //
> //===-------------------------------------------------------------------===//
> +#include "AsmParser/MipsAsmFlags.h"
> #include "MCTargetDesc/MipsELFStreamer.h"
> #include "MipsSubtarget.h"
> #include "llvm/MC/MCAssembler.h"
> @@ -74,6 +75,59 @@
> MCA.setELFHeaderEFlags(EFlags);
> }
>
> + // For llvm-mc. Set a group of ELF header flags
> + void
> + MipsELFStreamer::emitELFHeaderFlagsAsm(const MipsMCAsmFlags &Flags) {
> +
> + // Update e_header flags
> + MCAssembler& MCA = getAssembler();
> + unsigned EFlags = MCA.getELFHeaderEFlags();
> +
> + if (Flags.isAseMips16())
> + EFlags |= ELF::EF_MIPS_ARCH_ASE_M16;
> + else if (Flags.isAseMicroMips())
> + EFlags |= ELF::EF_MIPS_ARCH_ASE_M16;
> + else
> + EFlags |= ELF::EF_MIPS_NOREORDER;
> +
> + // Architecture
> + if (Flags.isArch64r2())
> + EFlags |= ELF::EF_MIPS_ARCH_64R2;
> + else if (Flags.isArch64r1())
> + EFlags |= ELF::EF_MIPS_ARCH_64;
> + else if (Flags.isArch32r2())
> + EFlags |= ELF::EF_MIPS_ARCH_32R2;
> + else
> + EFlags |= ELF::EF_MIPS_ARCH_32;
> +
> + if (Flags.isAseMicroMips())
> + EFlags |= ELF::EF_MIPS_MICROMIPS;
> +
> + // ABI
> + if (Flags.isAbiO32() || Flags.isAbiDefault())
> + EFlags |= ELF::EF_MIPS_ABI_O32;
> +// else if (Flags.isAbiN32())
> +// EFlags |= ELF::EF_MIPS_ABI2;
> +// else if (Flags.isAbiN64())
> +// EFlags |= ELF::EF_MIPS_ABI2;
Please remove commented out code.
> + else
> + llvm_unreachable("Unsupported ABI for e_flags");
> +
> + // Relocation Model
> + // TODO: Need to add -mabicalls and -mno-abicalls flags.
> + // Currently we assume that -mabicalls is the default.
> + if (Flags.isModelCpic() || Flags.isModelDefault())
> + EFlags |= ELF::EF_MIPS_CPIC;
> + if (Flags.isModelPic() || Flags.isModelDefault())
> + EFlags |= ELF::EF_MIPS_PIC;
> + else if (Flags.isModelStatic())
> + ; // Do nothing for Reloc::Static
> + else
> + llvm_unreachable("Unsupported relocation model for e_flags");
> +
> + MCA.setELFHeaderEFlags(EFlags);
> + }
> +
> // For llc. Set a symbol's STO flags
> void
> MipsELFStreamer::emitMipsSTOCG(const MipsSubtarget &Subtarget,
> Index: lib/Target/Mips/MCTargetDesc/MipsELFStreamer.h
> ===================================================================
> --- lib/Target/Mips/MCTargetDesc/MipsELFStreamer.h (revision 188764)
> +++ lib/Target/Mips/MCTargetDesc/MipsELFStreamer.h (working copy)
> @@ -13,6 +13,7 @@
>
> namespace llvm {
> class MipsAsmPrinter;
> +class MipsMCAsmFlags;
> class MipsSubtarget;
> class MCSymbol;
>
> @@ -26,6 +27,7 @@
>
> ~MipsELFStreamer() {}
> void emitELFHeaderFlagsCG(const MipsSubtarget &Subtarget);
> + void emitELFHeaderFlagsAsm(const MipsMCAsmFlags &MAFlags);
> void emitMipsSTOCG(const MipsSubtarget &Subtarget,
> MCSymbol *Sym,
> unsigned Val);
On Aug 20, 2013, at 7:28 AM, Vladimir Medic <Vladimir.Medic at imgtec.com> wrote:
> The assembler effects changes to the ELF environment through commandline
> options and inline directives. These need to be written out into the ELF
> header's eflags.
>
> The mechanism for this is through the MCELFStreamer. We have modeled the
> assembler's version after the previously implemented codegen one.
>
> I'm looking forward to your comments and remarks
>
> Vladimir
> <MipsElfFlags.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130826/261ee61e/attachment.html>
More information about the llvm-commits
mailing list