[TableGen][AsmMatcherEmitter] Only parse tokens following separators as registers

[ab]

Summary

This fixes PR23455, where, when TableGen generates the matcher from the AsmString, it splits "cmp${cc}ss" into tokens, and the "ss" suffix is recognized as the SS register.

I can't think of a situation where that's a feature, not a bug, hence this patch: when a token doesn't follow a separator (and is part of a "word"; wording better than "NewTok" much appreciated), it shouldn't be parsed as a register.

Thoughts?

-Ahmed

[craig.topper]

Does this still allow the problem with a hypothetical string of ss${cc}cmp?

utils/TableGen/AsmMatcherEmitter.cpp

971 ↗

(On Diff #26030)

separator

[ab]

In D9844#174841, @craig.topper wrote:

Does this still allow the problem with a hypothetical string of ss${cc}cmp?

Indeed it does; we can look in both directions, see new patch.

• Look at all the separators, not just space and comma.
• Add the PR testcase.

There's a special case for '$$', because Mips uses '$$reg', as '$' is the RegisterPrefix, and the best way of escaping it is '$$'. '\\$reg' will tokenize into separate '$' and 'reg', which isn't OK.
Changing that to not consider '\' a separator is a whole 'nother can of worms, as that leads to behavior changes on:

"\\{$Vd, $dst2, $dst3, $dst4\\}"

where we now think "dst4\}" is a single token.

[• tstellarAMD]

This seems useful. LGTM.

llvm/trunk/test/MC/X86/intel-syntax.s

 fsave dword ptr [eax]
 fxrstor dword ptr [eax]
 frstor dword ptr [eax]
 // CHECK: fxsave (%eax)
 // CHECK: wait
 // CHECK: fnsave (%eax)
 // CHECK: fxrstor (%eax)
 // CHECK: frstor (%eax)
+
+// CHECK: cmpnless %xmm1, %xmm0
+cmpnless xmm0, xmm1

llvm/trunk/utils/TableGen/AsmMatcherEmitter.cpp

struct AsmOperand {
     ClassInfo *Class;
 
     /// The operand name this is, if anything.
     StringRef SrcOpName;
 
     /// The suboperand index within SrcOpName, or -1 for the entire operand.
     int SubOpIdx;
 
+    /// Whether the token is "isolated", i.e., it is preceded and followed
+    /// by separators.
+    bool IsIsolatedToken;
+
     /// Register record if this token is singleton register.
     Record *SingletonReg;
 
-    explicit AsmOperand(StringRef T) : Token(T), Class(nullptr), SubOpIdx(-1),
-                                       SingletonReg(nullptr) {}
+    explicit AsmOperand(bool IsIsolatedToken, StringRef T)
+        : Token(T), Class(nullptr), SubOpIdx(-1),
+          IsIsolatedToken(IsIsolatedToken), SingletonReg(nullptr) {}
   };
 
   /// ResOperand - This represents a single operand in the result instruction
   /// generated by the match.  In cases (like addressing modes) where a single
   /// assembler operand expands to multiple MCOperands, this represents the
   /// single assembler operand, not the MCOperand.
   struct ResOperand {
     enum {
void MatchableInfo::initialize(const AsmMatcherInfo &Info,
 
   HasDeprecation =
       DepMask ? !DepMask->getValue()->getAsUnquotedString().empty() : false;
 }
 
 /// Append an AsmOperand for the given substring of AsmString.
 void MatchableInfo::addAsmOperand(size_t Start, size_t End) {
   StringRef String = AsmString;
-  AsmOperands.push_back(AsmOperand(String.slice(Start, End)));
+  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 =
+      (!Start || Separators.find(String[Start - 1]) != StringRef::npos ||
+       String.substr(Start - 1, 2) == "$$") &&
+      (End >= String.size() || Separators.find(String[End]) != StringRef::npos);
+  AsmOperands.push_back(AsmOperand(IsIsolatedToken, String.slice(Start, End)));
 }
 
 /// tokenizeAsmString - Tokenize a simplified assembly string.
 void MatchableInfo::tokenizeAsmString(const AsmMatcherInfo &Info) {
   StringRef String = AsmString;
   unsigned Prev = 0;
   bool InTok = true;
   for (unsigned i = 0, e = String.size(); i != e; ++i) {

 
 /// extractSingletonRegisterForAsmOperand - Extract singleton register,
 /// if present, from specified token.
 void MatchableInfo::
 extractSingletonRegisterForAsmOperand(unsigned OperandNo,
                                       const AsmMatcherInfo &Info,
                                       std::string &RegisterPrefix) {
   StringRef Tok = AsmOperands[OperandNo].Token;
+
+  // If this token is not an isolated token, i.e., it isn't separated from
+  // other tokens (e.g. with whitespace), don't interpret it as a register name.
+  if (!AsmOperands[OperandNo].IsIsolatedToken)
+    return;
+
   if (RegisterPrefix.empty()) {
     std::string LoweredTok = Tok.lower();
     if (const CodeGenRegister *Reg = Info.Target.getRegisterByName(LoweredTok))
       AsmOperands[OperandNo].SingletonReg = Reg->TheDef;
     return;
   }
 
   if (!Tok.startswith(RegisterPrefix))
buildInstructionOperandReference(MatchableInfo *II,
   if (Op->SubOpIdx == -1 && Operands[Idx].MINumOperands > 1) {
     Record *Rec = Operands[Idx].Rec;
     assert(Rec->isSubClassOf("Operand") && "Unexpected operand!");
     Record *MatchClass = Rec->getValueAsDef("ParserMatchClass");
     if (MatchClass && MatchClass->getValueAsString("Name") == "Imm") {
       // Insert remaining suboperands after AsmOpIdx in II->AsmOperands.
       StringRef Token = Op->Token; // save this in case Op gets moved
       for (unsigned SI = 1, SE = Operands[Idx].MINumOperands; SI != SE; ++SI) {
-        MatchableInfo::AsmOperand NewAsmOp(Token);
+        MatchableInfo::AsmOperand NewAsmOp(/*IsIsolatedToken=*/true, Token);
         NewAsmOp.SubOpIdx = SI;
         II->AsmOperands.insert(II->AsmOperands.begin()+AsmOpIdx+SI, NewAsmOp);
       }
       // Replace Op with first suboperand.
       Op = &II->AsmOperands[AsmOpIdx]; // update the pointer in case it moved
       Op->SubOpIdx = 0;
     }
   }