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

[Jason Kim]

Jim,

Thanks very much for the helpful comments.

Quick recap for Jim G:

All simple white space diffs have been quashed.
All non apropos comments made by me have been removed.
create(X86|ARM)MCStreamer have been "reverted" back to the clashing
static names.
Itemwise responses below:

Everyone:

This patch replaces the controversial arm-mc-elf-s01-r114621.patch I
sent to this
list earlier TODAY. It incorporates most of Jim G's suggestions (which
were originally for the
obsoleted arm-mc-elf-s01.patch4 sent earlier this week)

Patch applies cleanly to r114626, and make check-lit passes (yes
PR8199 is STILL not triggered :-).

Thank you.

-Jason

Notes-------------------

Towards eventual ARM/MC ELF emission
Its still mostly stub work so far.
Some cleanup in ARM Target Machine initialization - including
1. The ARM DataLayout member has been lifted upwards and initialized
directly via new method in ARMSubTarget
2. new ARMSubtarget::getDataLayout() method being called in ARMBaseTargetMachine


On Wed, Sep 22, 2010 at 6:48 PM, Jim Grosbach <grosbach at apple.com> wrote:
> 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.

When examining assembly (as I had to do for sanity checking), its
helpful to have matching names.


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

Done.  asserting.

<snip>
>      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?

I updated this portion of the code to better track what the how the
X86 initializes the DataLayout variable.


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

Uhm, I claim this is beyond the scope of this one patch. How about we
let this one slide for now, for the sake of consistency? :-)

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

comment reverted for more apropos patch.

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

Uhh, I am not sure yet about the addend. ARM arch manual does mention
it being allowed.
You are right about the is64bit. returns false for now.

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

Consider me warned :-)

-Jason
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arm-mc-elf-s01-r114626.patch4
Type: application/octet-stream
Size: 12456 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100922/e51a1c80/attachment.obj>