[PATCH] D56287: [X86] Fix incomplete handling of register-assigned variables in parsing.

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

Could use some test-cases for the error parsees, not just the successful parses.



================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:1950
   }
+  default: {
+    // This a memory operand or a register. We have some parsing complications
----------------
Since this is quite long, it may make sense to put the whole thing into ParseMemOperand -- perhaps renaming 'ParseX86MemOperandOrRegister'.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:1971
+    unsigned SegReg = 0;
+    if (getLexer().isNot(AsmToken::LParen)) {
+      // Parse Potential Imm. This is either an expression or a register.
----------------
I think it'd be clearer to create a temporary name e.g. "Expr" to use instead of "Imm" inside this block, for the expression value which is either an imm or register. And only assign to Imm in the "else" of the dyn_cast<X86MCExpr> conditional.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:1975
+        return nullptr;
+      // Parser out Register.
+      if (auto *RE = dyn_cast<X86MCExpr>(Imm)) {
----------------
Typo -> "Parse out"


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2020
+      bool AtMemOperand = false;
+      if (getLexer().is(AsmToken::LParen)) {
+        AsmToken Buf[2];
----------------
This entire block should be split into a separate "isAtMemOperand" helper function.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2023
+        auto TokCount = getLexer().peekTokens(Buf, true);
+        if (TokCount >= 1) {
+          StringRef Id;
----------------
I'd write > 0 here, the `>= 1` vs the `> 1` below confused me for a second.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2030
+            break;
+          case AsmToken::At:
+          case AsmToken::Dollar:
----------------
These lower cases are effectively implementing a "peekIdentifier" operation. It would be good to have a comment to that effect. Or, maybe peekIdentifier should even be split out as a separate function.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2061
+          return nullptr;
+        assert(!isa<X86MCExpr>(Imm) &&
+               "Expected non-register to be parsed here.");
----------------
Is this guaranteed to be true?


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2070-2071
+    // At this point we are either at the end of the operand or at the start of
+    // the base-index-scale-expr. If we're not at a "(" this is an immediate
+    // expression.
+
----------------
"If we're not at a "(" this is an immediate expression." is confusing, because it sounds like "this" is the upcoming tokens, not those already parsed. Probably can just be removed as redundant witht he first sentence.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2090
+      if (Parser.parseExpression(E, EndLoc) || !isa<X86MCExpr>(E))
+        return nullptr;
+
----------------
No error message if !isa<X86MCExpr>?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56287/new/

https://reviews.llvm.org/D56287