[PATCH] MIR Serialization: Serialize machine instruction names.

[Ahmed Bougacha]

On Wed, Jun 17, 2015 at 4:12 PM, Alex L <arphaman at gmail.com> wrote:
>
>
> 2015-06-17 15:25 GMT-07:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:
>>
>>
>> > On 2015 Jun 16, at 10:42, Alex Lorenz <arphaman at gmail.com> wrote:
>> >
>> > Hi dexonsmith, bob.wilson, bogner,
>> >
>> > This patch is based on a patch that implements initial serialization of
>> > machine basic blocks (http://reviews.llvm.org/D10465).
>> >
>> > This patch is the initial patch for machine instruction serialization.
>> > Only the machine instruction names are serialized by this patch. The
>> > instructions are represented using a YAML sequence of string literals and
>> > are a part of machine basic block YAML mapping. An example of such MBB
>> > mapping is shown below:
>> >
>> >  name: entry
>> >  instructions:
>> >    - mov32r0
>> >    - retq
>> >
>> > A machine instruction parser class 'MIParser' is introduced by this
>> > patch. This class will be used to parse the machine instructions. A
>> > supporting machine instruction lexing class will be added in the upcoming
>> > patch.
>> >
>> > REPOSITORY
>> >  rL LLVM
>> >
>> > http://reviews.llvm.org/D10481
>> >
>> > Files:
>> >  include/llvm/CodeGen/MIRYamlMapping.h
>> >  lib/CodeGen/MIRParser/CMakeLists.txt
>> >  lib/CodeGen/MIRParser/MIParser.cpp
>> >  lib/CodeGen/MIRParser/MIParser.h
>> >  lib/CodeGen/MIRParser/MIRParser.cpp
>> >  lib/CodeGen/MIRPrinter.cpp
>> >  test/CodeGen/MIR/X86/lit.local.cfg
>> >  test/CodeGen/MIR/X86/machine-instructions.mir
>> >  test/CodeGen/MIR/X86/unknown-instruction.mir
>> >
>> > EMAIL PREFERENCES
>> >  http://reviews.llvm.org/settings/panel/emailpreferences/
>> > <D10481.27769.patch>
>>
>> LGTM with a few minor nitpicks, although I know you're waiting on the
>> MBB parsing.
>>
>> > Index: lib/CodeGen/MIRParser/MIParser.cpp
>> > ===================================================================
>> > --- /dev/null
>> > +++ lib/CodeGen/MIRParser/MIParser.cpp
>> > @@ -0,0 +1,105 @@
>> > +//===- MIParser.cpp - Machine instructions parser implementation
>> > ----------===//
>> > +//
>> > +//                     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 parsing of machine instructions.
>> > +//
>> >
>> > +//===----------------------------------------------------------------------===//
>> > +
>> > +#include "MIParser.h"
>> > +#include "llvm/ADT/StringMap.h"
>> > +#include "llvm/CodeGen/MachineBasicBlock.h"
>> > +#include "llvm/CodeGen/MachineFunction.h"
>> > +#include "llvm/CodeGen/MachineInstr.h"
>> > +#include "llvm/Support/raw_ostream.h"
>> > +#include "llvm/Support/SourceMgr.h"
>> > +#include "llvm/Target/TargetSubtargetInfo.h"
>> > +#include "llvm/Target/TargetInstrInfo.h"
>> > +
>> > +using namespace llvm;
>> > +
>> > +namespace {
>> > +
>> > +class MIParser {
>> > +  SourceMgr &SM;
>> > +  MachineFunction &MF;
>> > +  SMDiagnostic &Error;
>> > +  StringRef Source;
>> > +  /// Maps from instruction names to op codes.
>> > +  StringMap<unsigned> Names2InstrOpCodes;
>> > +
>> > +public:
>> > +  MIParser(SourceMgr &SM, MachineFunction &MF, SMDiagnostic &Error,
>> > +           StringRef Source);
>> > +
>> > +  /// Report an error at the current location with the given message.
>> > +  ///
>> > +  /// This function always return true.
>> > +  bool error(const Twine &Msg);
>> > +
>> > +  MachineInstr *parse();
>> > +
>> > +private:
>> > +  void initNames2InstrOpCodes();
>> > +
>> > +  /// Try to convert an instruction name to an opcode. Return true if
>> > the
>> > +  /// instruction name is invalid.
>> > +  bool parseInstrName(StringRef InstrName, unsigned &OpCode);
>> > +};
>> > +
>> > +} // end anonymous namespace
>> > +
>> > +MIParser::MIParser(SourceMgr &SM, MachineFunction &MF, SMDiagnostic
>> > &Error,
>> > +                   StringRef Source)
>> > +    : SM(SM), MF(MF), Error(Error), Source(Source) {}
>> > +
>> > +bool MIParser::error(const Twine &Msg) {
>> > +  // TODO: Get the proper location in the MIR file, not just a location
>> > inside
>> > +  // the string.
>> > +  Error =
>> > +      SMDiagnostic(SM, SMLoc(), SM.getMemoryBuffer(SM.getMainFileID())
>> > +                                    ->getBufferIdentifier(),
>> > +                   1, 0, SourceMgr::DK_Error, Msg.str(), Source, None,
>> > None);
>> > +  return true;
>> > +}
>> > +
>> > +MachineInstr *MIParser::parse() {
>> > +  StringRef InstrName = Source;
>> > +  unsigned OpCode;
>> > +  if (parseInstrName(InstrName, OpCode)) {
>> > +    error(Twine("unknown machine instruction name '") + InstrName +
>> > "'");
>> > +    return nullptr;
>> > +  }
>>
>> We need a TODO somewhere (maybe here?) for the rest of the instruction ;).
>>
>> > +
>> > +  const auto &MCID = MF.getSubtarget().getInstrInfo()->get(OpCode);
>> > +  auto *MI = MF.CreateMachineInstr(MCID, DebugLoc());
>> > +  return MI;
>> > +}
>> > +
>> > +void MIParser::initNames2InstrOpCodes() {
>> > +  if (!Names2InstrOpCodes.empty())
>> > +    return;
>> > +  const auto *TII = MF.getSubtarget().getInstrInfo();
>>
>> Something like:
>>
>>     assert(TII && "Expected target instruction info");
>>
>> might be nice.
>>
>> > +  for (unsigned I = 0, E = TII->getNumOpcodes(); I < E; ++I)
>> > +    Names2InstrOpCodes.insert(
>> > +        std::make_pair(StringRef(TII->getName(I)).lower(), I));
>> > +}
>> > +
>> > +bool MIParser::parseInstrName(StringRef InstrName, unsigned &OpCode) {
>> > +  initNames2InstrOpCodes();
>> > +  auto InstrInfo = Names2InstrOpCodes.find(InstrName);
>>
>> Should we support other cases, using `InstrName.lower()`, here?  (I'm
>> not sure we should; really just a question.  I'm happy to go ahead with
>> what you have, since it's easy to loosen later.)
>
>
> I'm not sure really, I just print and parse everything using lowercase name,
> like LLVM IR.
> I think this can be changed later if other people want case insensitive
> format.

Ah, here's the email thread ;)

I think case sensitivity makes more sense, and might be necessary
(matching tablegen, IIRC).  All else being equal, please do that
instead!

-Ahmed

>>
>>
>> > +  if (InstrInfo == Names2InstrOpCodes.end())
>> > +    return true;
>> > +  OpCode = InstrInfo->getValue();
>> > +  return false;
>> > +}
>> > +
>> > +MachineInstr *llvm::parseMachineInstr(SourceMgr &SM, MachineFunction
>> > &MF,
>> > +                                      StringRef Src, SMDiagnostic
>> > &Error) {
>> > +  return MIParser(SM, MF, Error, Src).parse();
>> > +}
>>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>