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

Jim Grosbach grosbach at apple.com
Fri Sep 17 11:48:18 PDT 2010


Applied, with a couple of very minor tweaks, as r114195. Thanks for the patch!

-Jim

On Sep 16, 2010, at 1:54 PM, Jason Kim wrote:

> Hi Jim.
> 
> Thanks for the feedback. Much appreciated.
> Here's the revised patch.
> 
> 
> On Thu, Sep 16, 2010 at 11:19 AM, Jim Grosbach <grosbach at apple.com> wrote:
>> Hi Jason,
>> 
>> Glad to see this moving forward! This looks like a great start. A few minor comments on the patch itself inline below. Apologies in advance for being nitpicky.
>> 
>> I don't recall whether we covered this bit specifically before, so at the risk of repeating myself... On a general note, it seems to me that a reasonable first milestone would be to have the target independent layer recognize that the ARM target wants to support object file emission. That is, have llc recognize "-filetype=obj". Right now it issues an error "target does not support generation of this file type!" (which is, of course, entirely correct for the moment). Once the classes are in place to do that, you'll start hitting all of the placeholder asserts(), which is exactly what you want, as you can interactively follow more easily the path the code wants to follow for simple cases and fill in the bits that as you go.
>> 
> 
> Sounds like a plan! Up for that next.
> 
>> 
>>> Index: lib/Target/ARM/ARM.h
>>> ===================================================================
>>> --- lib/Target/ARM/ARM.h      (revision 114081)
>>> +++ lib/Target/ARM/ARM.h      (working copy)
>>> @@ -26,7 +26,14 @@
>>>  class FunctionPass;
>>>  class JITCodeEmitter;
>>>  class formatted_raw_ostream;
>>> +class MCCodeEmitter;
>>> 
>>> +
>>> +MCCodeEmitter *createARM_MCCodeEmitter(const Target &,
>> 
>> As a style thing, LLVM doesn't use underscores in symbol names. Just createARMMCCodeEmitter() is fine. (The underscore in the X86 equivalent is there as part of the target name, X86_32 vs. X86_64, and so is a bit misleading in this regard).
>> 
>> 
>>> +                                       TargetMachine &TM,
>>> +                                       MCContext &Ctx);
>>> +
>>> +
>> 
>> Extra vertical whitespace here can be removed.
>> 
>>>  FunctionPass *createARMISelDag(ARMBaseTargetMachine &TM,
>>>                                 CodeGenOpt::Level OptLevel);
>>> 
>>> @@ -41,6 +48,9 @@
>>>  FunctionPass *createThumb2ITBlockPass();
>>>  FunctionPass *createThumb2SizeReductionPass();
>>> 
>>> +
>>> +
>>> +
>> 
>> Ditto. There's other instances below that can be similarly cleaned up.
>> 
>>>  extern Target TheARMTarget, TheThumbTarget;
>>> 
>>>  } // end namespace llvm;
>>> Index: lib/Target/ARM/ARMMCCodeEmitter.cpp
>>> ===================================================================
>>> --- lib/Target/ARM/ARMMCCodeEmitter.cpp       (revision 0)
>>> +++ lib/Target/ARM/ARMMCCodeEmitter.cpp       (revision 0)
>>> @@ -0,0 +1,134 @@
>>> +//===-- ARM/ARMMCCodeEmitter.cpp - Convert ARM code to machine code -------===//
>>> +//
>>> +//                     The LLVM Compiler Infrastructure
>>> +//
>>> +// This file is distributed under the University of Illinois Open Source
>>> +// License. See LICENSE.TXT for details.
>>> +//
>>> +//===----------------------------------------------------------------------===//
>>> +//
>>> +// This file implements the ARMMCCodeEmitter class.
>>> +//
>>> +//===----------------------------------------------------------------------===//
>>> +
>>> +#define DEBUG_TYPE "ARM-emitter"
>> 
>> Lower-case here, i.e., "arm-emitter" is more consistent with other options of this sort.
>> 
>>> +#include "ARM.h"
>>> +#include "ARMInstrInfo.h"
>>> +//#include "ARMFixupKinds.h"
>>> +#include "llvm/MC/MCCodeEmitter.h"
>>> +#include "llvm/MC/MCExpr.h"
>>> +#include "llvm/MC/MCInst.h"
>>> +#include "llvm/Support/raw_ostream.h"
>>> +using namespace llvm;
>>> +
>>> +namespace {
>>> +class ARMMCCodeEmitter : public MCCodeEmitter {
>>> +  ARMMCCodeEmitter(const ARMMCCodeEmitter &); // DO NOT IMPLEMENT
>>> +  void operator=(const ARMMCCodeEmitter &); // DO NOT IMPLEMENT
>>> +  const TargetMachine &TM;
>>> +  const TargetInstrInfo &TII;
>>> +  MCContext &Ctx;
>>> +  bool Is64BitMode;
>> 
>> Can just remove the extra hold-over bool from the x86 bit here. No 64-bit mode on ARM. :)
>> 
>>> +public:
>>> +  ARMMCCodeEmitter(TargetMachine &tm, MCContext &ctx)
>>> +    : TM(tm), TII(*TM.getInstrInfo()), Ctx(ctx) {
>>> +  }
>>> +
>>> +  ~ARMMCCodeEmitter() {}
>>> +
>>> +  unsigned getNumFixupKinds() const {
>>> +    assert(0 && "ARMCodeEmitter will be implemented soon");
>>> +  }
>>> +
>>> +  const MCFixupKindInfo &getFixupKindInfo(MCFixupKind Kind) const {
>>> +    static MCFixupKindInfo rtn;
>>> +    assert(0 && "ARMCodeEmitter will be implemented soon");
>> 
>> Totally not a big deal, but I tend to phrase these sorts of things as simply "<function name> not yet implemented." Personal preference to simply state accurately the status of things rather than indicate anything about the future. Plus, then when I see the assert, I know specifically which function is being hit, rather than just the general name of the feature.
>> 
>>> +    return rtn;
>>> +  }
>>> +
>>> +  static unsigned GetARMRegNum(const MCOperand &MO) {
>>> +    assert(0 && "ARMCodeEmitter will be implemented soon");
>>> +    return 0;
>>> +  }
>>> +
>>> +  void EmitByte(unsigned char C, unsigned &CurByte, raw_ostream &OS) const {
>>> +    OS << (char)C;
>>> +    ++CurByte;
>>> +  }
>>> +
>>> +  void EmitConstant(uint64_t Val, unsigned Size, unsigned &CurByte,
>>> +                    raw_ostream &OS) const {
>>> +    assert(0 && "ARMCodeEmitter will be implemented soon");
>> 
>> Is this assert necessary? I don't know any reason why the implementation below wouldn't be sufficient.
>> 
>>> +    // Output the constant in little endian byte order.
>>> +    for (unsigned i = 0; i != Size; ++i) {
>>> +      EmitByte(Val & 255, CurByte, OS);
>>> +      Val >>= 8;
>>> +    }
>>> +  }
>>> +
>>> +  void EmitImmediate(const MCOperand &Disp,
>>> +                     unsigned ImmSize, MCFixupKind FixupKind,
>>> +                     unsigned &CurByte, raw_ostream &OS,
>>> +                     SmallVectorImpl<MCFixup> &Fixups,
>>> +                     int ImmOffset = 0) const;
>>> +
>>> +
>>> +
>>> +
>>> +
>>> +  void EncodeInstruction(const MCInst &MI, raw_ostream &OS,
>>> +                         SmallVectorImpl<MCFixup> &Fixups) const;
>>> +
>>> +  void EmitOpcodePrefix(uint64_t TSFlags, unsigned &CurByte, int MemOperand,
>>> +                        const MCInst &MI, const TargetInstrDesc &Desc,
>>> +                        raw_ostream &OS) const;
>>> +};
>>> +
>>> +} // end anonymous namespace
>>> +
>>> +
>>> +MCCodeEmitter *llvm::createARM_MCCodeEmitter(const Target &,
>>> +                                             TargetMachine &TM,
>>> +                                             MCContext &Ctx) {
>>> +  return new ARMMCCodeEmitter(TM, Ctx);
>>> +}
>>> +
>>> +
>>> +
>>> +/// getImmFixupKind - Return the appropriate fixup kind to use for an immediate
>>> +/// in an instruction with the specified TSFlags.
>>> +static MCFixupKind getImmFixupKind(uint64_t TSFlags) {
>>> +  static MCFixupKind rtn;
>>> +  assert(0 && "ARMCodeEmitter will be implemented soon");
>>> +  return rtn;
>>> +}
>>> +
>>> +
>>> +void ARMMCCodeEmitter::
>>> +EmitImmediate(const MCOperand &DispOp, unsigned Size, MCFixupKind FixupKind,
>>> +              unsigned &CurByte, raw_ostream &OS,
>>> +              SmallVectorImpl<MCFixup> &Fixups, int ImmOffset) const {
>>> +  assert(0 && "ARMCodeEmitter will be implemented soon");
>>> +}
>>> +
>>> +
>>> +
>>> +
>>> +
>>> +
>>> +/// EmitOpcodePrefix - Emit all instruction prefixes prior to the opcode.
>>> +///
>>> +/// MemOperand is the operand # of the start of a memory operand if present.  If
>>> +/// Not present, it is -1.
>>> +void ARMMCCodeEmitter::EmitOpcodePrefix(uint64_t TSFlags, unsigned &CurByte,
>>> +                                        int MemOperand, const MCInst &MI,
>>> +                                        const TargetInstrDesc &Desc,
>>> +                                        raw_ostream &OS) const {
>>> +  assert(0 && "ARMCodeEmitter will be implemented soon");
>>> +}
>>> +
>>> +void ARMMCCodeEmitter::
>>> +EncodeInstruction(const MCInst &MI, raw_ostream &OS,
>>> +                  SmallVectorImpl<MCFixup> &Fixups) const {
>>> +  assert(0 && "ARMCodeEmitter will be implemented soon");
>>> +}
>>> Index: lib/Target/ARM/CMakeLists.txt
>>> ===================================================================
>>> --- lib/Target/ARM/CMakeLists.txt     (revision 114081)
>>> +++ lib/Target/ARM/CMakeLists.txt     (working copy)
>>> @@ -28,6 +28,7 @@
>>>    ARMISelLowering.cpp
>>>    ARMInstrInfo.cpp
>>>    ARMJITInfo.cpp
>>> +  ARMMCCodeEmitter.cpp
>>>    ARMLoadStoreOptimizer.cpp
>>>    ARMMCAsmInfo.cpp
>>>    ARMMCInstLower.cpp
>>> 
>> 
> <arm-mc-elf.patch3>





More information about the llvm-commits mailing list