[AsmParser] Backends can parameterize ASM tokenization.

[colinl]

Summary

This allows backends to specify how to lex their instructions through variables in the .TD files.

Several backends need to do special things with the dot character and Hexagon contains numerous additional characters that need to be tokenized #()=:.<>!+*.

[colinl]

252439

[chandlerc]

And here Colin is yet another commit you have landed without review.

You seem to have been landing multiple MC changes without getting any
review or comment from anyone else working on MC despite mailing them out
for pre-commit review. What on earth is going on here?

Looking at the diff, this also seems to effectively revert multiple changes
made to the AsmParser code in the last few months. I assume you're trying
to upstream patches from some internal branch, but you *really must*
actually merge the trunk changes in!

Yet again, please revert this, and every other patch dependent on it if
necessary, and every other patch you are landing outside of Hexagon without
bothering to respect our code review process.

-Chandler

[colinl]

I see the size_t loop variable was mistakenly changed to an unsigned.

Were there other reverted changes I should make note of? My hope was for this patch to result in no functional change by not changing any tests and ensuring the all passed after the patch.

[craig.topper]

It also changed from StringRef::find back to std::find

[echristo]

How do you see this as working. The changes you made to the existing ports don't show how this would be useful.

What's going on here? I think there have been enough concerns over this patch set that you might want to temporarily revert and send out a design doc for what you want to change.

-eric

[kparzysz]

There is a review for the revert: D15874.

[colinl]

The change motivation is because TableGen and the AsmParser recognize different token sets which caused them to tokenize in fundamentally different ways. Before this change if TableGen was presented with "if(p0) r0 = r0" it would tokenize it as { "if(p0)", "r0", "=", "r0" } and the AsmParser would tokenize it as { "if", "(", "p0", ")", "r0", "=', "r0" } Before this patch the set of tokens that are part of identifiers was fixed by the tokenizeAsmString switch table shared by all targets. The first inclination is to add the parenthesis tokens to the switch table but this causes issues in other targets, for instance in X86 the ST(0) register has a hard time being parsed with this change which means different targets have mutually exclusive tokenization rules hence the allowance for targets to specify their tokens.

The existing tokenizeAsmString did three different variants of tokenization. It discarded certain separator characters "a,b" -> { "a", "b" } It would tokenize certain characters "a$b" -> { "a", "$", "b" } and it did a weird one-off break of the dot character but only sometimes and would concatenate it to the following identifier "a.b" -> { "a", ".b" } The change allowed these to be parameterized by each target, something we were already kind of doing for the dot break case with "MnemonicContainsDot" which this patch replaced and something that needed to be done for the mutually exclusive parsing rules.

Merge mistakes aside I haven't heard a proposal or volunteer to make larger design changes to the assembly parser.

include/llvm/Target/Target.td

   // register tokens as constrained registers, instead of tokens, for the
   // purposes of matching.
   string RegisterPrefix = "";
+
+  // TokenizingCharacters - Characters that are standalone tokens
+  string TokenizingCharacters = "[]*!";
+
+  // SeparatorCharacters - Characters that are not tokens
+  string SeparatorCharacters = " \t,";
+
+  // BreakCharacters - Characters that start new identifiers
+  string BreakCharacters = "";
 }
 def DefaultAsmParserVariant : AsmParserVariant;
 

lib/Target/AArch64/AArch64.td

 def GenericAsmParserVariant : AsmParserVariant {
   int Variant = 0;
   string Name = "generic";
+  string BreakCharacters = ".";
 }
 
 def AppleAsmParserVariant : AsmParserVariant {
   int Variant = 1;
   string Name = "apple-neon";
+  string BreakCharacters = ".";
 }
 
 //===----------------------------------------------------------------------===//

lib/Target/ARM/ARM.td

   bit isMCAsmWriter = 1;
 }
 
+def ARMAsmParserVariant : AsmParserVariant {
+  int Variant = 0;
+  string Name = "ARM";
+  string BreakCharacters = ".";
+}
+
 def ARM : Target {
   // Pull in Instruction Info:
   let InstructionSet = ARMInstrInfo;
   let AssemblyWriters = [ARMAsmWriter];
+  let AssemblyParserVariants = [ARMAsmParserVariant];
 }

lib/Target/BPF/BPF.td

   bit isMCAsmWriter = 1;
 }
 
+def BPFAsmParserVariant : AsmParserVariant {
+  int Variant = 0;
+  string Name = "BPF";
+  string BreakCharacters = ".";
+}
+
 def BPF : Target {
   let InstructionSet = BPFInstrInfo;
   let AssemblyWriters = [BPFInstPrinter];
+  let AssemblyParserVariants = [BPFAsmParserVariant];
 }

lib/Target/PowerPC/PPC.td

   // InstAlias definitions use immediate literals.  Set RegisterPrefix
   // so that those are not misinterpreted as registers.
   string RegisterPrefix = "%";
+  string BreakCharacters = ".";
 }
 
 def PPC : Target {

utils/TableGen/AsmMatcherEmitter.cpp

   }
 };
 
+class AsmVariantInfo {
+public:
+  std::string TokenizingCharacters;
+  std::string SeparatorCharacters;
+  std::string BreakCharacters;
+};
+
 /// MatchableInfo - Helper class for storing the necessary information for an
 /// instruction or alias which is capable of being matched.
 struct MatchableInfo {

 
   void initialize(const AsmMatcherInfo &Info,
                   SmallPtrSetImpl<Record*> &SingletonRegisters,
-                  int AsmVariantNo, StringRef RegisterPrefix);
+                  int AsmVariantNo, StringRef RegisterPrefix,
+                  AsmVariantInfo const &Variant);
 
   /// validate - Return true if this matchable is a valid thing to match against
   /// and perform a bunch of validity checking.

   void dump() const;
 
 private:
-  void tokenizeAsmString(const AsmMatcherInfo &Info);
-  void addAsmOperand(size_t Start, size_t End);
+  void tokenizeAsmString(AsmMatcherInfo const &Info,
+                         AsmVariantInfo const &Variant);
+  void addAsmOperand(size_t Start, size_t End,
+                     std::string const &SeparatorCharacters);
 };
 
 /// SubtargetFeatureInfo - Helper class for storing information on a subtarget

 
 void MatchableInfo::initialize(const AsmMatcherInfo &Info,
                                SmallPtrSetImpl<Record*> &SingletonRegisters,
-                               int AsmVariantNo, StringRef RegisterPrefix) {
+                               int AsmVariantNo, StringRef RegisterPrefix,
+                               AsmVariantInfo const &Variant) {
   AsmVariantID = AsmVariantNo;
   AsmString =
     CodeGenInstruction::FlattenAsmStringVariants(AsmString, AsmVariantNo);
 
-  tokenizeAsmString(Info);
+  tokenizeAsmString(Info, Variant);
 
   // Compute the require features.
   for (Record *Predicate : TheDef->getValueAsListOfDefs("Predicates"))

 }
 
 /// Append an AsmOperand for the given substring of AsmString.
-void MatchableInfo::addAsmOperand(size_t Start, size_t End) {
+void MatchableInfo::addAsmOperand(size_t Start, size_t End,
+                                  std::string const &Separators) {
   StringRef String = AsmString;
-  StringRef Separators = "[]*! \t,";
   // Look for separators before and after to figure out is this token is
   // isolated.  Accept '$$' as that's how we escape '$'.
   bool IsIsolatedToken =

 }
 
 /// tokenizeAsmString - Tokenize a simplified assembly string.
-void MatchableInfo::tokenizeAsmString(const AsmMatcherInfo &Info) {
+void MatchableInfo::tokenizeAsmString(const AsmMatcherInfo &Info,
+                                      AsmVariantInfo const &Variant) {
   StringRef String = AsmString;
-  size_t Prev = 0;
-  bool InTok = true;
-  for (size_t i = 0, e = String.size(); i != e; ++i) {
-    switch (String[i]) {
-    case '[':
-    case ']':
-    case '*':
-    case '!':
-    case ' ':
-    case '\t':
-    case ',':
-      if (InTok) {
-        addAsmOperand(Prev, i);
+  unsigned Prev = 0;
+  bool InTok = false;
+  std::string Separators = Variant.TokenizingCharacters +
+                           Variant.SeparatorCharacters;
+  for (unsigned i = 0, e = String.size(); i != e; ++i) {
+    if(Variant.BreakCharacters.find(String[i]) != std::string::npos) {
+      if(InTok) {
+        addAsmOperand(Prev, i, Separators);
+        Prev = i;
+      }
+      InTok = true;
+      continue;
+    }
+    if(Variant.TokenizingCharacters.find(String[i]) != std::string::npos) {
+      if(InTok) {
+        addAsmOperand(Prev, i, Separators);
         InTok = false;
       }
-      if (!isspace(String[i]) && String[i] != ',')
-        addAsmOperand(i, i + 1);
+      addAsmOperand(i, i + 1, Separators);
       Prev = i + 1;
-      break;
-
+      continue;
+    }
+    if(Variant.SeparatorCharacters.find(String[i]) != std::string::npos) {
+      if(InTok) {
+        addAsmOperand(Prev, i, Separators);
+        InTok = false;
+      }
+      Prev = i + 1;
+      continue;
+    }
+    switch (String[i]) {
     case '\\':
       if (InTok) {
-        addAsmOperand(Prev, i);
+        addAsmOperand(Prev, i, Separators);
         InTok = false;
       }
       ++i;
       assert(i != String.size() && "Invalid quoted character");
-      addAsmOperand(i, i + 1);
+      addAsmOperand(i, i + 1, Separators);
       Prev = i + 1;
       break;
 
     case '$': {
-      if (InTok) {
-        addAsmOperand(Prev, i);
+      if (InTok && Prev != i) {
+        addAsmOperand(Prev, i, Separators);
         InTok = false;
       }
 

         break;
       }
 
-      // If this is "${" find the next "}" and make an identifier like "${xxx}"
-      size_t EndPos = String.find('}', i);
-      assert(EndPos != StringRef::npos &&
-             "Missing brace in operand reference!");
-      addAsmOperand(i, EndPos+1);
+      StringRef::iterator End = std::find(String.begin() + i, String.end(),'}');
+      assert(End != String.end() && "Missing brace in operand reference!");
+      size_t EndPos = End - String.begin();
+      addAsmOperand(i, EndPos+1, Separators);
       Prev = EndPos + 1;
       i = EndPos;
       break;
     }
-
-    case '.':
-      if (!Info.AsmParser->getValueAsBit("MnemonicContainsDot")) {
-        if (InTok)
-          addAsmOperand(Prev, i);
-        Prev = i;
-      }
-      InTok = true;
-      break;
-
     default:
       InTok = true;
     }
   }
   if (InTok && Prev != String.size())
-    addAsmOperand(Prev, StringRef::npos);
+    addAsmOperand(Prev, StringRef::npos, Separators);
 
   // The first token of the instruction is the mnemonic, which must be a
   // simple string, not a $foo variable or a singleton register.

     std::string CommentDelimiter =
       AsmVariant->getValueAsString("CommentDelimiter");
     std::string RegisterPrefix = AsmVariant->getValueAsString("RegisterPrefix");
+    AsmVariantInfo Variant;
+    Variant.TokenizingCharacters =
+        AsmVariant->getValueAsString("TokenizingCharacters");
+    Variant.SeparatorCharacters =
+        AsmVariant->getValueAsString("SeparatorCharacters");
+    Variant.BreakCharacters =
+        AsmVariant->getValueAsString("BreakCharacters");
     int AsmVariantNo = AsmVariant->getValueAsInt("Variant");
 
     for (const CodeGenInstruction *CGI : Target.instructions()) {

 
       auto II = llvm::make_unique<MatchableInfo>(*CGI);
 
-      II->initialize(*this, SingletonRegisters, AsmVariantNo, RegisterPrefix);
+      II->initialize(*this, SingletonRegisters, AsmVariantNo, RegisterPrefix,
+                     Variant);
 
       // Ignore instructions which shouldn't be matched and diagnose invalid
       // instruction definitions with an error.

 
       auto II = llvm::make_unique<MatchableInfo>(std::move(Alias));
 
-      II->initialize(*this, SingletonRegisters, AsmVariantNo, RegisterPrefix);
+      II->initialize(*this, SingletonRegisters, AsmVariantNo, RegisterPrefix,
+                     Variant);
 
       // Validate the alias definitions.
       II->validate(CommentDelimiter, false);