MIR Parser: Simplify the token's string value handling.

[arphaman]

Summary

This patch removes the StringOffset and HasStringValue fields from the MIToken struct and simplifies the stringValue method which now returns the new StringValue field.

[silvas]

Except for removing rawStringValue this entire suggestion was pretty nitpicky anyway, so feel free to ignore if any of this doesn't seem worth it.

lib/CodeGen/MIRParser/MILexer.h

113 ↗

(On Diff #31422)

It seems weird to me that we are adding an operator= (and a non-obvious one at that) for a class that is never really being assigned. It seems like the complexity is coming from the use of MIToken constructor to reinitialize an MIToken. I think it would be simpler to use two explicit calls:

• resetToken(Kind, Range)

then one of

• setIntVal(APSInt &)
• setStrValNonOwned(StringRef) (or use an offset; but I like the symmetry with Owned)
• setStrValOwned(StringRef/std::string) (copies into internal buffer and sets StrVal)

Conveniently all the existing uses of the MIToken constructors like this are already broken across two lines, so this should not increase the size of the reinitialization source code (but allows deleting code duplicated across these 3 constructors).

An even simpler solution is to just use std::string/SmallString and have it always own (not sure how performance-critical this is). Once it grows large enough there should be no further need to reallocate, so it is just a small memcpy cost.

[arphaman]

The updated patch now uses the 'reset' and 'set...' pattern instead of MIToken's constructors as suggested by Sean.

[silvas]

Nice! I wasn't expecting a builder pattern but that turns out really nice! LGTM

[silvas]

Mark as "Accept" in Phab. (don't think it really matters, but just to be consistent with the LGTM in the previous comment).

llvm/trunk/lib/CodeGen/MIRParser/MILexer.h

enum TokenKind {
     JumpTableIndex,
     NamedIRBlock,
     IRBlock,
     NamedIRValue,
   };
 
 private:
   TokenKind Kind;
-  unsigned StringOffset;
-  bool HasStringValue;
   StringRef Range;
-  std::string StringValue;
+  StringRef StringValue;
+  std::string StringValueStorage;
   APSInt IntVal;
 
 public:
-  MIToken(TokenKind Kind, StringRef Range, unsigned StringOffset = 0)
-      : Kind(Kind), StringOffset(StringOffset), HasStringValue(false),
-        Range(Range) {}
-
-  MIToken(TokenKind Kind, StringRef Range, std::string StringValue,
-          unsigned StringOffset = 0)
-      : Kind(Kind), StringOffset(StringOffset), HasStringValue(true),
-        Range(Range), StringValue(std::move(StringValue)) {}
-
-  MIToken(TokenKind Kind, StringRef Range, const APSInt &IntVal,
-          unsigned StringOffset = 0)
-      : Kind(Kind), StringOffset(StringOffset), HasStringValue(false),
-        Range(Range), IntVal(IntVal) {}
+  MIToken() : Kind(Error) {}
+
+  MIToken &reset(TokenKind Kind, StringRef Range);
+
+  MIToken &setStringValue(StringRef StrVal);
+  MIToken &setOwnedStringValue(std::string StrVal);
+  MIToken &setIntegerValue(APSInt IntVal);
 
   TokenKind kind() const { return Kind; }
 
   bool isError() const { return Kind == Error; }
 
   bool isRegister() const {
     return Kind == NamedRegister || Kind == underscore ||
            Kind == VirtualRegister;
public:
   }
 
   bool is(TokenKind K) const { return Kind == K; }
 
   bool isNot(TokenKind K) const { return Kind != K; }
 
   StringRef::iterator location() const { return Range.begin(); }
 
-  /// Return the token's raw string value.
-  ///
-  /// If the string value is quoted, this method returns that quoted string as
-  /// it is, without unescaping the string value.
-  StringRef rawStringValue() const { return Range.drop_front(StringOffset); }
+  StringRef range() const { return Range; }
 
   /// Return the token's string value.
-  StringRef stringValue() const {
-    return HasStringValue ? StringRef(StringValue)
-                          : Range.drop_front(StringOffset);
-  }
+  StringRef stringValue() const { return StringValue; }
 
   const APSInt &integerValue() const { return IntVal; }
 
   bool hasIntegerValue() const {
     return Kind == IntegerLiteral || Kind == MachineBasicBlock ||
            Kind == StackObject || Kind == FixedStackObject ||
            Kind == GlobalValue || Kind == VirtualRegister ||
            Kind == ConstantPoolItem || Kind == JumpTableIndex ||

llvm/trunk/lib/CodeGen/MIRParser/MILexer.cpp

public:
 
   StringRef::iterator location() const { return Ptr; }
 
   operator bool() const { return Ptr != nullptr; }
 };
 
 } // end anonymous namespace
 
+MIToken &MIToken::reset(TokenKind Kind, StringRef Range) {
+  this->Kind = Kind;
+  this->Range = Range;
+  return *this;
+}
+
+MIToken &MIToken::setStringValue(StringRef StrVal) {
+  StringValue = StrVal;
+  return *this;
+}
+
+MIToken &MIToken::setOwnedStringValue(std::string StrVal) {
+  StringValueStorage = std::move(StrVal);
+  StringValue = StringValueStorage;
+  return *this;
+}
+
+MIToken &MIToken::setIntegerValue(APSInt IntVal) {
+  this->IntVal = std::move(IntVal);
+  return *this;
+}
+
 /// Skip the leading whitespace characters and return the updated cursor.
 static Cursor skipWhitespace(Cursor C) {
   while (isspace(C.peek()))
     C.advance();
   return C;
 }
 
 /// Return true if the given character satisfies the following regular

 static Cursor lexName(
     Cursor C, MIToken &Token, MIToken::TokenKind Type, unsigned PrefixLength,
     function_ref<void(StringRef::iterator Loc, const Twine &)> ErrorCallback) {
   auto Range = C;
   C.advance(PrefixLength);
   if (C.peek() == '"') {
     if (Cursor R = lexStringConstant(C, ErrorCallback)) {
       StringRef String = Range.upto(R);
-      Token = MIToken(Type, String,
-                      unescapeQuotedString(String.drop_front(PrefixLength)),
-                      PrefixLength);
+      Token.reset(Type, String)
+          .setOwnedStringValue(
+              unescapeQuotedString(String.drop_front(PrefixLength)));
       return R;
     }
-    Token = MIToken(MIToken::Error, Range.remaining());
+    Token.reset(MIToken::Error, Range.remaining());
     return Range;
   }
   while (isIdentifierChar(C.peek()))
     C.advance();
-  Token = MIToken(Type, Range.upto(C), PrefixLength);
+  Token.reset(Type, Range.upto(C))
+      .setStringValue(Range.upto(C).drop_front(PrefixLength));
   return C;
 }
 
 static Cursor maybeLexIntegerType(Cursor C, MIToken &Token) {
   if (C.peek() != 'i' || !isdigit(C.peek(1)))
     return None;
   auto Range = C;
   C.advance(); // Skip 'i'
   while (isdigit(C.peek()))
     C.advance();
-  Token = MIToken(MIToken::IntegerType, Range.upto(C));
+  Token.reset(MIToken::IntegerType, Range.upto(C));
   return C;
 }
 
 static MIToken::TokenKind getIdentifierKind(StringRef Identifier) {
   return StringSwitch<MIToken::TokenKind>(Identifier)
       .Case("_", MIToken::underscore)
       .Case("implicit", MIToken::kw_implicit)
       .Case("implicit-def", MIToken::kw_implicit_define)

 
 static Cursor maybeLexIdentifier(Cursor C, MIToken &Token) {
   if (!isalpha(C.peek()) && C.peek() != '_' && C.peek() != '.')
     return None;
   auto Range = C;
   while (isIdentifierChar(C.peek()))
     C.advance();
   auto Identifier = Range.upto(C);
-  Token = MIToken(getIdentifierKind(Identifier), Identifier);
+  Token.reset(getIdentifierKind(Identifier), Identifier)
+      .setStringValue(Identifier);
   return C;
 }
 
 static Cursor maybeLexMachineBasicBlock(
     Cursor C, MIToken &Token,
     function_ref<void(StringRef::iterator Loc, const Twine &)> ErrorCallback) {
   if (!C.remaining().startswith("%bb."))
     return None;
   auto Range = C;
   C.advance(4); // Skip '%bb.'
   if (!isdigit(C.peek())) {
-    Token = MIToken(MIToken::Error, C.remaining());
+    Token.reset(MIToken::Error, C.remaining());
     ErrorCallback(C.location(), "expected a number after '%bb.'");
     return C;
   }
   auto NumberRange = C;
   while (isdigit(C.peek()))
     C.advance();
   StringRef Number = NumberRange.upto(C);
   unsigned StringOffset = 4 + Number.size(); // Drop '%bb.<id>'
   if (C.peek() == '.') {
     C.advance(); // Skip '.'
     ++StringOffset;
     while (isIdentifierChar(C.peek()))
       C.advance();
   }
-  Token = MIToken(MIToken::MachineBasicBlock, Range.upto(C), APSInt(Number),
-                  StringOffset);
+  Token.reset(MIToken::MachineBasicBlock, Range.upto(C))
+      .setIntegerValue(APSInt(Number))
+      .setStringValue(Range.upto(C).drop_front(StringOffset));
   return C;
 }
 
 static Cursor maybeLexIndex(Cursor C, MIToken &Token, StringRef Rule,
                             MIToken::TokenKind Kind) {
   if (!C.remaining().startswith(Rule) || !isdigit(C.peek(Rule.size())))
     return None;
   auto Range = C;
   C.advance(Rule.size());
   auto NumberRange = C;
   while (isdigit(C.peek()))
     C.advance();
-  Token = MIToken(Kind, Range.upto(C), APSInt(NumberRange.upto(C)));
+  Token.reset(Kind, Range.upto(C)).setIntegerValue(APSInt(NumberRange.upto(C)));
   return C;
 }
 
 static Cursor maybeLexIndexAndName(Cursor C, MIToken &Token, StringRef Rule,
                                    MIToken::TokenKind Kind) {
   if (!C.remaining().startswith(Rule) || !isdigit(C.peek(Rule.size())))
     return None;
   auto Range = C;
   C.advance(Rule.size());
   auto NumberRange = C;
   while (isdigit(C.peek()))
     C.advance();
   StringRef Number = NumberRange.upto(C);
   unsigned StringOffset = Rule.size() + Number.size();
   if (C.peek() == '.') {
     C.advance();
     ++StringOffset;
     while (isIdentifierChar(C.peek()))
       C.advance();
   }
-  Token = MIToken(Kind, Range.upto(C), APSInt(Number), StringOffset);
+  Token.reset(Kind, Range.upto(C))
+      .setIntegerValue(APSInt(Number))
+      .setStringValue(Range.upto(C).drop_front(StringOffset));
   return C;
 }
 
 static Cursor maybeLexJumpTableIndex(Cursor C, MIToken &Token) {
   return maybeLexIndex(C, Token, "%jump-table.", MIToken::JumpTableIndex);
 }
 
 static Cursor maybeLexStackObject(Cursor C, MIToken &Token) {

 }
 
 static Cursor lexVirtualRegister(Cursor C, MIToken &Token) {
   auto Range = C;
   C.advance(); // Skip '%'
   auto NumberRange = C;
   while (isdigit(C.peek()))
     C.advance();
-  Token = MIToken(MIToken::VirtualRegister, Range.upto(C),
-                  APSInt(NumberRange.upto(C)));
+  Token.reset(MIToken::VirtualRegister, Range.upto(C))
+      .setIntegerValue(APSInt(NumberRange.upto(C)));
   return C;
 }
 
 static Cursor maybeLexRegister(Cursor C, MIToken &Token) {
   if (C.peek() != '%')
     return None;
   if (isdigit(C.peek(1)))
     return lexVirtualRegister(C, Token);
   auto Range = C;
   C.advance(); // Skip '%'
   while (isIdentifierChar(C.peek()))
     C.advance();
-  Token = MIToken(MIToken::NamedRegister, Range.upto(C),
-                  /*StringOffset=*/1); // Drop the '%'
+  Token.reset(MIToken::NamedRegister, Range.upto(C))
+      .setStringValue(Range.upto(C).drop_front(1)); // Drop the '%'
   return C;
 }
 
 static Cursor maybeLexGlobalValue(
     Cursor C, MIToken &Token,
     function_ref<void(StringRef::iterator Loc, const Twine &)> ErrorCallback) {
   if (C.peek() != '@')
     return None;
   if (!isdigit(C.peek(1)))
     return lexName(C, Token, MIToken::NamedGlobalValue, /*PrefixLength=*/1,
                    ErrorCallback);
   auto Range = C;
   C.advance(1); // Skip the '@'
   auto NumberRange = C;
   while (isdigit(C.peek()))
     C.advance();
-  Token =
-      MIToken(MIToken::GlobalValue, Range.upto(C), APSInt(NumberRange.upto(C)));
+  Token.reset(MIToken::GlobalValue, Range.upto(C))
+      .setIntegerValue(APSInt(NumberRange.upto(C)));
   return C;
 }
 
 static Cursor maybeLexExternalSymbol(
     Cursor C, MIToken &Token,
     function_ref<void(StringRef::iterator Loc, const Twine &)> ErrorCallback) {
   if (C.peek() != '$')
     return None;
static Cursor maybeLexHexFloatingPointLiteral(Cursor C, MIToken &Token) {
   if (C.peek() != '0' || C.peek(1) != 'x')
     return None;
   Cursor Range = C;
   C.advance(2); // Skip '0x'
   if (isValidHexFloatingPointPrefix(C.peek()))
     C.advance();
   while (isxdigit(C.peek()))
     C.advance();
-  Token = MIToken(MIToken::FloatingPointLiteral, Range.upto(C));
+  Token.reset(MIToken::FloatingPointLiteral, Range.upto(C));
   return C;
 }
 
 static Cursor lexFloatingPointLiteral(Cursor Range, Cursor C, MIToken &Token) {
   C.advance();
   // Skip over [0-9]*([eE][-+]?[0-9]+)?
   while (isdigit(C.peek()))
     C.advance();
   if ((C.peek() == 'e' || C.peek() == 'E') &&
       (isdigit(C.peek(1)) ||
        ((C.peek(1) == '-' || C.peek(1) == '+') && isdigit(C.peek(2))))) {
     C.advance(2);
     while (isdigit(C.peek()))
       C.advance();
   }
-  Token = MIToken(MIToken::FloatingPointLiteral, Range.upto(C));
+  Token.reset(MIToken::FloatingPointLiteral, Range.upto(C));
   return C;
 }
 
 static Cursor maybeLexNumericalLiteral(Cursor C, MIToken &Token) {
   if (!isdigit(C.peek()) && (C.peek() != '-' || !isdigit(C.peek(1))))
     return None;
   auto Range = C;
   C.advance();
   while (isdigit(C.peek()))
     C.advance();
   if (C.peek() == '.')
     return lexFloatingPointLiteral(Range, C, Token);
   StringRef StrVal = Range.upto(C);
-  Token = MIToken(MIToken::IntegerLiteral, StrVal, APSInt(StrVal));
+  Token.reset(MIToken::IntegerLiteral, StrVal).setIntegerValue(APSInt(StrVal));
   return C;
 }
 
 static MIToken::TokenKind symbolToken(char C) {
   switch (C) {
   case ',':
     return MIToken::comma;
   case '=':
if (C.peek() == ':' && C.peek(1) == ':') {
     Kind = MIToken::coloncolon;
     Length = 2;
   } else
     Kind = symbolToken(C.peek());
   if (Kind == MIToken::Error)
     return None;
   auto Range = C;
   C.advance(Length);
-  Token = MIToken(Kind, Range.upto(C));
+  Token.reset(Kind, Range.upto(C));
   return C;
 }
 
 StringRef llvm::lexMIToken(
     StringRef Source, MIToken &Token,
     function_ref<void(StringRef::iterator Loc, const Twine &)> ErrorCallback) {
   auto C = skipWhitespace(Cursor(Source));
   if (C.isEOF()) {
-    Token = MIToken(MIToken::Eof, C.remaining());
+    Token.reset(MIToken::Eof, C.remaining());
     return C.remaining();
   }
 
   if (Cursor R = maybeLexIntegerType(C, Token))
     return R.remaining();
   if (Cursor R = maybeLexIdentifier(C, Token))
     return R.remaining();
   if (Cursor R = maybeLexMachineBasicBlock(C, Token, ErrorCallback))
if (Cursor R = maybeLexExternalSymbol(C, Token, ErrorCallback))
     return R.remaining();
   if (Cursor R = maybeLexHexFloatingPointLiteral(C, Token))
     return R.remaining();
   if (Cursor R = maybeLexNumericalLiteral(C, Token))
     return R.remaining();
   if (Cursor R = maybeLexSymbol(C, Token))
     return R.remaining();
 
-  Token = MIToken(MIToken::Error, C.remaining());
+  Token.reset(MIToken::Error, C.remaining());
   ErrorCallback(C.location(),
                 Twine("unexpected character '") + Twine(C.peek()) + "'");
   return C.remaining();
 }

llvm/trunk/lib/CodeGen/MIRParser/MIParser.cpp

 };
 
 } // end anonymous namespace
 
 MIParser::MIParser(SourceMgr &SM, MachineFunction &MF, SMDiagnostic &Error,
                    StringRef Source, const PerFunctionMIParsingState &PFS,
                    const SlotMapping &IRSlots)
     : SM(SM), MF(MF), Error(Error), Source(Source), CurrentSource(Source),
-      Token(MIToken::Error, StringRef()), PFS(PFS), IRSlots(IRSlots) {}
+      PFS(PFS), IRSlots(IRSlots) {}
 
 void MIParser::lex() {
   CurrentSource = lexMIToken(
       CurrentSource, Token,
       [this](StringRef::iterator Loc, const Twine &Msg) { error(Loc, Msg); });
 }
 
 bool MIParser::error(const Twine &Msg) { return error(Token.location(), Msg); }
bool MIParser::parseImmediateOperand(MachineOperand &Dest) {
   if (Int.getMinSignedBits() > 64)
     return error("integer literal is too large to be an immediate operand");
   Dest = MachineOperand::CreateImm(Int.getExtValue());
   lex();
   return false;
 }
 
 bool MIParser::parseIRConstant(StringRef::iterator Loc, const Constant *&C) {
-  auto Source = StringRef(Loc, Token.stringValue().end() - Loc).str();
+  auto Source = StringRef(Loc, Token.range().end() - Loc).str();
   lex();
   SMDiagnostic Err;
   C = parseConstantValue(Source.c_str(), Err, *MF.getFunction()->getParent());
   if (!C)
     return error(Loc + Err.getColumnNo(), Err.getMessage());
   return false;
 }
 

 }
 
 bool MIParser::parseGlobalValue(GlobalValue *&GV) {
   switch (Token.kind()) {
   case MIToken::NamedGlobalValue: {
     const Module *M = MF.getFunction()->getParent();
     GV = M->getNamedValue(Token.stringValue());
     if (!GV)
-      return error(Twine("use of undefined global value '@") +
-                   Token.rawStringValue() + "'");
+      return error(Twine("use of undefined global value '") + Token.range() +
+                   "'");
     break;
   }
   case MIToken::GlobalValue: {
     unsigned GVIdx;
     if (getUnsigned(GVIdx))
       return true;
     if (GVIdx >= IRSlots.GlobalValues.size())
       return error(Twine("use of undefined global value '@") + Twine(GVIdx) +

 }
 
 bool MIParser::parseIRBlock(BasicBlock *&BB, const Function &F) {
   switch (Token.kind()) {
   case MIToken::NamedIRBlock: {
     BB = dyn_cast_or_null<BasicBlock>(
         F.getValueSymbolTable().lookup(Token.stringValue()));
     if (!BB)
-      return error(Twine("use of undefined IR block '%ir-block.") +
-                   Token.rawStringValue() + "'");
+      return error(Twine("use of undefined IR block '") + Token.range() + "'");
     break;
   }
   case MIToken::IRBlock: {
     unsigned SlotNumber = 0;
     if (getUnsigned(SlotNumber))
       return true;
     BB = const_cast<BasicBlock *>(getIRBlock(SlotNumber));
     if (!BB)
if (Dest.isReg())
     return error(Loc, "register operands can't have target flags");
   Dest.setTargetFlags(TF);
   return false;
 }
 
 bool MIParser::parseOperandsOffset(MachineOperand &Op) {
   if (Token.isNot(MIToken::plus) && Token.isNot(MIToken::minus))
     return false;
-  StringRef Sign = Token.stringValue();
+  StringRef Sign = Token.range();
   bool IsNegative = Token.is(MIToken::minus);
   lex();
   if (Token.isNot(MIToken::IntegerLiteral))
     return error("expected an integer literal after '" + Sign + "'");
   if (Token.integerValue().getMinSignedBits() > 64)
     return error("expected 64-bit integer (too large)");
   int64_t Offset = Token.integerValue().getExtValue();
   if (IsNegative)
     Offset = -Offset;
   lex();
   Op.setOffset(Offset);
   return false;
 }
 
 bool MIParser::parseIRValue(Value *&V) {
   switch (Token.kind()) {
   case MIToken::NamedIRValue: {
     V = MF.getFunction()->getValueSymbolTable().lookup(Token.stringValue());
     if (!V)
-      return error(Twine("use of undefined IR value '%ir.") +
-                   Token.rawStringValue() + "'");
+      return error(Twine("use of undefined IR value '") + Token.range() + "'");
     break;
   }
   // TODO: Parse unnamed IR value references.
   default:
     llvm_unreachable("The current token should be an IR block reference");
   }
   return false;
 }