[PATCH] D13592: [MC] Fixed parsing of macro arguments where expressions with spaces are present.

[Daniel Sanders via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.

> In order to pass the newly added test cases, I had to produce another patch to fix issues whilst printing expressions. This patch requires this in order to pass all test cases. The patch ca be found here: http://reviews.llvm.org/D15949


Hmm. The code changed by that patch already breaks the encapsulation of the MCExpr hierarchy and needs to be moved inside MipsMCExpr. I tried doing this a few weeks ago and it's a lot harder than it looks because there's two mutually exclusive ways to handle things like %hi and we seem to have chosen both. http://reviews.llvm.org/D15949 makes the encapsulation worse by duplicating a functionality from MCExpr so I'd rather not commit that and instead work on putting the current contents of that function inside the MCExpr hierarchy.

Dropping the tests involving 'sym' doesn't fundamentally change the testing w.r.t whitespace separated arguments so I think the lesser evil is to remove the sym tests from this patch. On balance, I think we should go with this patch by itself and drop the tests marked below.

LGTM with the two removals indicated below.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3906-3920
@@ -3905,2 +3905,17 @@
     return true;
+  case AsmToken::Identifier: {
+    StringRef Identifier;
+    if (Parser.parseIdentifier(Identifier))
+      return true;
+
+    SMLoc S = Parser.getTok().getLoc();
+    SMLoc E = SMLoc::getFromPointer(S.getPointer());
+    MCSymbol *Sym = getContext().getOrCreateSymbol(Identifier);
+    // Otherwise create a symbol reference.
+    const MCExpr *Res =
+        MCSymbolRefExpr::create(Sym, MCSymbolRefExpr::VK_None, getContext());
+
+    Operands.push_back(MipsOperand::CreateImm(Res, S, E, *this));
+    return false;
+  }
   case AsmToken::Dollar: {
----------------
Given that we're dropping the symbol test cases for now, we should drop this section too and leave it for a later patch

================
Comment at: test/MC/Mips/user-macro-argument-separation.s:41-70
@@ +40,31 @@
+nop                          # CHECK: nop
+EX2 addiu $2, sym $3         # CHECK: addiu    $2, $3, sym
+EX2 addiu $2, -sym $3        # CHECK: addiu    $2, $3, -sym
+EX2 addiu $2, - sym $3       # CHECK: addiu    $2, $3, -sym
+EX2 addiu $2, 1+sym $3       # CHECK: addiu    $2, $3, 1+sym
+EX2 addiu $2, 1+ sym $3      # CHECK: addiu    $2, $3, 1+sym
+EX2 addiu $2, 1 +sym $3      # CHECK: addiu    $2, $3, 1+sym
+EX2 addiu $2, 1 + sym $3     # CHECK: addiu    $2, $3, 1+sym
+EX2 addiu $2, 1+~sym $3      # CHECK: addiu    $2, $3, 1+~sym
+EX2 addiu $2, 1+~ sym $3     # CHECK: addiu    $2, $3, 1+~sym
+EX2 addiu $2, 1+ ~sym $3     # CHECK: addiu    $2, $3, 1+~sym
+EX2 addiu $2, 1 +~sym $3     # CHECK: addiu    $2, $3, 1+~sym
+EX2 addiu $2, 1 +~ sym $3    # CHECK: addiu    $2, $3, 1+~sym
+EX2 addiu $2, 1 + ~sym $3    # CHECK: addiu    $2, $3, 1+~sym
+EX2 addiu $2, 1 + ~ sym $3   # CHECK: addiu    $2, $3, 1+~sym
+EX2 addiu $2, 1+(sym) $3     # CHECK: addiu    $2, $3, 1+sym
+# CHECK: fixup A - offset: 0, value: sym, kind: fixup_Mips_32
+EX2 addiu $2, 1 +(sym) $3    # CHECK: addiu    $2, $3, 1+sym
+# CHECK: fixup A - offset: 0, value: sym, kind: fixup_Mips_32
+EX2 addiu $2, 1+ (sym) $3    # CHECK: addiu    $2, $3, 1+sym
+# CHECK: fixup A - offset: 0, value: sym, kind: fixup_Mips_32
+EX2 addiu $2, 1 + (sym) $3   # CHECK: addiu    $2, $3, 1+sym
+# CHECK: fixup A - offset: 0, value: sym, kind: fixup_Mips_32
+EX2 addiu $2, 1+(1)+sym $3   # CHECK: addiu    $2, $3, 2+sym
+# CHECK: fixup A - offset: 0, value: sym, kind: fixup_Mips_32
+EX2 addiu $2, 1 +(1)+sym $3  # CHECK: addiu    $2, $3, 2+sym
+# CHECK: fixup A - offset: 0, value: sym, kind: fixup_Mips_32
+EX2 addiu $2, 1+ (1)+sym $3  # CHECK: addiu    $2, $3, 2+sym
+# CHECK: fixup A - offset: 0, value: sym, kind: fixup_Mips_32
+EX2 addiu $2, 1 + (1)+sym $3 # CHECK: addiu    $2, $3, 2+sym
+# CHECK: fixup A - offset: 0, value: sym, kind: fixup_Mips_32
----------------
As explained above, I'm not keen on this suggestion but it's the lesser of two evils. Could you remove these tests so that we don't need D15949?


http://reviews.llvm.org/D13592