[llvm-commits] Initial cut of ARM MC ELF emitter (PATCH)

Jason Kim jasonwkim at google.com
Wed Sep 22 20:55:24 PDT 2010


Hi Jim,

Just makes debugging a little easier at times, is all (explicit
names), makes it easier to realize what function I am in.
Do you want me to revert the name change?

BTW, I *just* sent an update to this patch, it fixes a bug I introduced in it .

(sorry!) and Thanks

-jason


On Wed, Sep 22, 2010 at 6:48 PM, Jim Grosbach <grosbach at apple.com> wrote:
>
> On Sep 19, 2010, at 10:36 PM, Jason Kim wrote:
>
> Double oops.
> I somehow missed a compile breaking typo.
> Apologies for the noise.
> Tested against clean build dir.
> <arm-mc-elf-s01.patch4>
>
> Index: lib/Target/X86/X86TargetMachine.cpp
> ===================================================================
> --- lib/Target/X86/X86TargetMachine.cpp (revision 114214)
> +++ lib/Target/X86/X86TargetMachine.cpp (working copy)
> @@ -39,11 +39,11 @@
>    }
>  }
> -static MCStreamer *createMCStreamer(const Target &T, const std::string &TT,
> -                                    MCContext &Ctx, TargetAsmBackend &TAB,
> -                                    raw_ostream &_OS,
> -                                    MCCodeEmitter *_Emitter,
> -                                    bool RelaxAll) {
> +static MCStreamer *createX86MCStreamer(const Target &T, const std::string
> &TT,
> +                                       MCContext &Ctx, TargetAsmBackend
> &TAB,
> +                                       raw_ostream &_OS,
> +                                       MCCodeEmitter *_Emitter,
> +                                       bool RelaxAll) {
>
> Why is this rename necessary? It's a static function, so there's no
> namespace collision with anything in another target.
>
>    Triple TheTriple(TT);
>    switch (TheTriple.getOS()) {
>    case Triple::Darwin:
> @@ -58,7 +58,7 @@
>    }
>  }
> -extern "C" void LLVMInitializeX86Target() {
> +extern "C" void LLVMInitializeX86Target() {
>    // Register the target.
>    RegisterTargetMachine<X86_32TargetMachine> X(TheX86_32Target);
>    RegisterTargetMachine<X86_64TargetMachine> Y(TheX86_64Target);
> @@ -81,9 +81,9 @@
>    // Register the object streamer.
>    TargetRegistry::RegisterObjectStreamer(TheX86_32Target,
> -                                         createMCStreamer);
> +                                         createX86MCStreamer);
>    TargetRegistry::RegisterObjectStreamer(TheX86_64Target,
> -                                         createMCStreamer);
> +                                         createX86MCStreamer);
>  }
>
> @@ -100,9 +100,9 @@
>  /// X86TargetMachine ctor - Create an X86 target.
>  ///
> -X86TargetMachine::X86TargetMachine(const Target &T, const std::string &TT,
> +X86TargetMachine::X86TargetMachine(const Target &T, const std::string &TT,
>                                     const std::string &FS, bool is64Bit)
> -  : LLVMTargetMachine(T, TT),
> +  : LLVMTargetMachine(T, TT),
>      Subtarget(TT, FS, is64Bit),
>      DataLayout(Subtarget.getDataLayout()),
>      FrameInfo(TargetFrameInfo::StackGrowsDown,
> @@ -217,13 +217,13 @@
>                                        JITCodeEmitter &JCE) {
>    // FIXME: Move this to TargetJITInfo!
>    // On Darwin, do not override 64-bit setting made in X86TargetMachine().
> -  if (DefRelocModel == Reloc::Default &&
> +  if (DefRelocModel == Reloc::Default &&
>        (!Subtarget.isTargetDarwin() || !Subtarget.is64Bit())) {
>      setRelocationModel(Reloc::Static);
>      Subtarget.setPICStyle(PICStyles::None);
>    }
> -
> +
>
> Looks like a few whitespace only changes slipped into the patch. It's best
> if cosmetic changes and functional changes are done in separate patches to
> make it easier to track which revisions are making "real" changes to the
> code.
>
>    PM.add(createX86JITCodeEmitterPass(*this, JCE));
>    return false;
> Index: lib/Target/ARM/ARMTargetMachine.cpp
> ===================================================================
> --- lib/Target/ARM/ARMTargetMachine.cpp (revision 114214)
> +++ lib/Target/ARM/ARMTargetMachine.cpp (working copy)
> @@ -31,6 +31,27 @@
>    }
>  }
> +// TODO(jasonwkim): [2010/09/19 10:31:16 PDT (Sunday)]
> +// This is duplicated code. Refactor this.
> +static MCStreamer *createARMMCStreamer(const Target &T, const std::string
> &TT,
>
> It's static, so it doesn't technically need the "ARM" bit in the name. Not
> really a big deal, but I'd prefer changing things here to be consistent with
> X86 for now, then adjusting both later if it's more appropriate to include
> the prefix. That is, add new bits in a manner consistent with what's already
> there, then do the cleanup, or vice versa, but not both in the same patch.
> Same basic idea as above in that it keeps the patches small and single
> purpose, which makes them easier to test and review.
>
> +                                       MCContext &Ctx, TargetAsmBackend
> &TAB,
> +                                       raw_ostream &_OS,
> +                                       MCCodeEmitter *_Emitter,
> +                                       bool RelaxAll) {
> +  Triple TheTriple(TT);
> +  switch (TheTriple.getOS()) {
> +  case Triple::Darwin:
> +    return createMachOStreamer(Ctx, TAB, _OS, _Emitter, RelaxAll);
> +  case Triple::MinGW32:
> +  case Triple::MinGW64:
> +  case Triple::Cygwin:
> +  case Triple::Win32:
> +    return createWinCOFFStreamer(Ctx, TAB, *_Emitter, _OS, RelaxAll);
>
> ARM doesn't currently have any intention of supporting any of the Windows
> targets, so these should be omitted here, or possibly trigger an assert.
>
> +  default:
> +    return createELFStreamer(Ctx, TAB, _OS, _Emitter, RelaxAll);
> +  }
> +}
> +
>  extern "C" void LLVMInitializeARMTarget() {
>    // Register the target.
>    RegisterTargetMachine<ARMTargetMachine> X(TheARMTarget);
> @@ -39,6 +60,19 @@
>    // Register the target asm info.
>    RegisterAsmInfoFn A(TheARMTarget, createMCAsmInfo);
>    RegisterAsmInfoFn B(TheThumbTarget, createMCAsmInfo);
> +
> +  // Register the MC Code Emitter
> +  TargetRegistry::RegisterCodeEmitter(TheARMTarget,
> +                                      createARMMCCodeEmitter);
> +  TargetRegistry::RegisterCodeEmitter(TheThumbTarget,
> +                                      createARMMCCodeEmitter);
> +
> +  // Register the object streamer.
> +  TargetRegistry::RegisterObjectStreamer(TheARMTarget,
> +                                         createARMMCStreamer);
> +  TargetRegistry::RegisterObjectStreamer(TheThumbTarget,
> +                                         createARMMCStreamer);
> +
>  }
>  /// TargetMachine ctor - Create an ARM architecture model.
> @@ -46,23 +80,28 @@
>  ARMBaseTargetMachine::ARMBaseTargetMachine(const Target &T,
>                                             const std::string &TT,
>                                             const std::string &FS,
> +                                           StringRef TargetDescription,
>                                             bool isThumb)
>    : LLVMTargetMachine(T, TT),
>      Subtarget(TT, FS, isThumb),
>      FrameInfo(Subtarget),
>      JITInfo(),
> -    InstrItins(Subtarget.getInstrItineraryData()) {
> +    InstrItins(Subtarget.getInstrItineraryData()),
> +    DataLayout(TargetDescription),
>
>  The TargetData is accessible from the TargetMachine instance via the
> getTargetData() accessor function. Is that not sufficient for what you're
> looking to accomplish?
>
> +    ELFWriterInfo(*this)
>
> This creates an ELFWriter() even when the target doesn't need or want one.
> That doesn't seem right. I see X86 does the same thing, though. Doesn't seem
> right there, either, but perhaps I'm missing something and someone familiar
> with that area can comment.
>
> <snip>
>
> Index: lib/Target/ARM/ARMELFWriterInfo.h
> ===================================================================
> --- lib/Target/ARM/ARMELFWriterInfo.h (revision 0)
> +++ lib/Target/ARM/ARMELFWriterInfo.h (revision 0)
> @@ -0,0 +1,66 @@
> +//===-- ARMELFWriterInfo.h - ELF Writer Info for ARM ------------*- C++
> -*-===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This file implements ELF writer information for the ARM backend.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#ifndef ARM_ELF_WRITER_INFO_H
> +#define ARM_ELF_WRITER_INFO_H
> +
> +#include "llvm/Target/TargetELFWriterInfo.h"
> +
> +namespace llvm {
> +
> +  class ARMELFWriterInfo : public TargetELFWriterInfo {
> +
> +    // ELF Relocation types for ARM
> +    // FIXME: TODO(jasonwkim): [2010/09/17 14:52:25 PDT (Friday)]
>
> FIXME's don't need name/date info. "svn annotate" does the trick for that
> information.
>
> +    // Come up with a better way to orgnize the 100+ ARM reloc types.
> +
> +    enum ARMRelocationType {
> +    };
> +
> +  public:
> +    ARMELFWriterInfo(TargetMachine &TM);
> +    virtual ~ARMELFWriterInfo();
> +
> +    /// getRelocationType - Returns the target specific ELF Relocation
> type.
> +    /// 'MachineRelTy' contains the object code independent relocation type
> +    virtual unsigned getRelocationType(unsigned MachineRelTy) const;
> +
> +    /// hasRelocationAddend - True if the target uses an addend in the
> +    /// ELF relocation entry.
> +    virtual bool hasRelocationAddend() const { return is64Bit ? true :
> false; }
>
> There's no 64-bit here. Should this always return 'false' for ARM? Probably
> shouldn't reference 64Bit in any case, I'd think.
>
> +
> +    /// getDefaultAddendForRelTy - Gets the default addend value for a
> +    /// relocation entry based on the target ELF relocation type.
> +    virtual long int getDefaultAddendForRelTy(unsigned RelTy,
> +                                              long int Modifier = 0) const;
>
> +
> +    /// isPCRelativeRel - True if the relocation type is pc relative
> +    virtual bool isPCRelativeRel(unsigned RelTy) const;
> +
> +    /// getJumpTableRelocationTy - Returns the machine relocation type used
> +    /// to reference a jumptable.
> +    virtual unsigned getAbsoluteLabelMachineRelTy() const;
>
> Advance warning: ARM does jump tables with custom handling. This may be an
> "interesting" bit.
>
> +
> +    /// computeRelocation - Some relocatable fields could be relocated
> +    /// directly, avoiding the relocation symbol emission, compute the
> +    /// final relocation value for this symbol.
> +    virtual long int computeRelocation(unsigned SymOffset, unsigned
> RelOffset,
> +                                       unsigned RelTy) const;
> +  };
> +
> +} // end llvm namespace
> +
> +#endif // ARM_ELF_WRITER_INFO_H
> Index: lib/Target/ARM/ARMELFWriterInfo.cpp
> ===================================================================
> --- lib/Target/ARM/ARMELFWriterInfo.cpp (revision 0)
> +++ lib/Target/ARM/ARMELFWriterInfo.cpp (revision 0)
> @@ -0,0 +1,65 @@
> +//===-- ARMELFWriterInfo.cpp - ELF Writer Info for the ARM backend
> --------===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This file implements ELF writer information for the ARM backend.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#include "ARMELFWriterInfo.h"
> +#include "ARMRelocations.h"
> +#include "llvm/Function.h"
> +#include "llvm/Support/ErrorHandling.h"
> +#include "llvm/Target/TargetData.h"
> +#include "llvm/Target/TargetMachine.h"
> +
> +using namespace llvm;
> +
> +//===----------------------------------------------------------------------===//
> +//  Implementation of the ARMELFWriterInfo class
> +//===----------------------------------------------------------------------===//
> +
> +ARMELFWriterInfo::ARMELFWriterInfo(TargetMachine &TM)
> +  : TargetELFWriterInfo(TM) {
> +  // silently OK construction
> +}
> +
> +ARMELFWriterInfo::~ARMELFWriterInfo() {}
> +
> +unsigned ARMELFWriterInfo::getRelocationType(unsigned MachineRelTy) const {
> +  assert(0 && "ARMELFWriterInfo::getRelocationType() not implemented");
> +  return 0;
> +}
> +
> +long int ARMELFWriterInfo::getDefaultAddendForRelTy(unsigned RelTy,
> +                                                    long int Modifier)
> const {
> +  assert(0 && "ARMELFWriterInfo::getDefaultAddendForRelTy() not
> implemented");
> +  return 0;
> +}
> +
> +unsigned ARMELFWriterInfo::getRelocationTySize(unsigned RelTy) const {
> +  assert(0 && "ARMELFWriterInfo::getRelocationTySize() not implemented");
> +  return 0;
> +}
> +
> +bool ARMELFWriterInfo::isPCRelativeRel(unsigned RelTy) const {
> +  assert(0 && "ARMELFWriterInfo::isPCRelativeRel() not implemented");
> +  return 1;
> +}
> +
> +unsigned ARMELFWriterInfo::getAbsoluteLabelMachineRelTy() const {
> +  assert(0 && "ARMELFWriterInfo::getAbsoluteLabelMachineRelTy() not
> implemented");
> +  return 0;
> +}
> +
> +long int ARMELFWriterInfo::computeRelocation(unsigned SymOffset,
> +                                             unsigned RelOffset,
> +                                             unsigned RelTy) const {
> +  assert(0 && "ARMELFWriterInfo::getAbsoluteLabelMachineRelTy() not
> implemented");
> +  return 0;
> +}
> Index: lib/Target/ARM/CMakeLists.txt
> ===================================================================
> --- lib/Target/ARM/CMakeLists.txt (revision 114214)
> +++ lib/Target/ARM/CMakeLists.txt (working copy)
> @@ -21,6 +21,7 @@
>    ARMCodeEmitter.cpp
>    ARMConstantIslandPass.cpp
>    ARMConstantPoolValue.cpp
> +  ARMELFWriterInfo.cpp
>    ARMExpandPseudoInsts.cpp
>    ARMFastISel.cpp
>    ARMGlobalMerge.cpp
> Index: lib/Target/ARM/ARMTargetMachine.h
> ===================================================================
> --- lib/Target/ARM/ARMTargetMachine.h (revision 114214)
> +++ lib/Target/ARM/ARMTargetMachine.h (working copy)
> @@ -16,7 +16,9 @@
>  #include "llvm/Target/TargetMachine.h"
>  #include "llvm/Target/TargetData.h"
> +#include "llvm/MC/MCStreamer.h"
>  #include "ARMInstrInfo.h"
> +#include "ARMELFWriterInfo.h"
>  #include "ARMFrameInfo.h"
>  #include "ARMJITInfo.h"
>  #include "ARMSubtarget.h"
> @@ -38,10 +40,21 @@
>    InstrItineraryData  InstrItins;
>    Reloc::Model        DefRelocModel;    // Reloc model before it's
> overridden.
> +protected:
> +  const TargetData    DataLayout;       // Calculates type size & alignment
> +  ARMELFWriterInfo    ELFWriterInfo;
> +
>  public:
>    ARMBaseTargetMachine(const Target &T, const std::string &TT,
> -                       const std::string &FS, bool isThumb);
> +                       const std::string &FS,
> +                       StringRef Target,
> +                       bool isThumb);
> +  virtual const TargetData       *getTargetData() const { return
> &DataLayout; }
> +  virtual const ARMELFWriterInfo *getELFWriterInfo() const {
> +    return Subtarget.isTargetELF() ? &ELFWriterInfo : 0;
> +  };
> +
>    virtual const ARMFrameInfo     *getFrameInfo() const { return &FrameInfo;
> }
>    virtual       ARMJITInfo       *getJITInfo()         { return &JITInfo; }
>    virtual const ARMSubtarget  *getSubtargetImpl() const { return
> &Subtarget; }
> @@ -63,7 +76,6 @@
>  ///
>  class ARMTargetMachine : public ARMBaseTargetMachine {
>    ARMInstrInfo        InstrInfo;
> -  const TargetData    DataLayout;       // Calculates type size & alignment
>    ARMTargetLowering   TLInfo;
>    ARMSelectionDAGInfo TSInfo;
>  public:
> @@ -93,7 +105,6 @@
>  class ThumbTargetMachine : public ARMBaseTargetMachine {
>    // Either Thumb1InstrInfo or Thumb2InstrInfo.
>    OwningPtr<ARMBaseInstrInfo> InstrInfo;
> -  const TargetData    DataLayout;   // Calculates type size & alignment
>    ARMTargetLowering   TLInfo;
>    ARMSelectionDAGInfo TSInfo;
>  public:
> Index: lib/Target/TargetELFWriterInfo.cpp
> ===================================================================
> --- lib/Target/TargetELFWriterInfo.cpp (revision 114214)
> +++ lib/Target/TargetELFWriterInfo.cpp (working copy)
> @@ -18,6 +18,8 @@
>  using namespace llvm;
>  TargetELFWriterInfo::TargetELFWriterInfo(TargetMachine &tm) : TM(tm) {
> +  // TODO(jasonwkim): [2010/09/18 09:57:44 PDT (Saturday)]
> +  // Fixme: NaCl 64bit has 32bit pointers. Need a better test.
>    is64Bit = TM.getTargetData()->getPointerSizeInBits() == 64;
>    isLittleEndian = TM.getTargetData()->isLittleEndian();
>  }
>




More information about the llvm-commits mailing list