[PATCH] D92690: [MC][AMDGPU] Consume EndOfStatement in asm parser

[Scott Linder via Phabricator via llvm-commits]

scott.linder updated this revision to Diff 310363.
scott.linder added a comment.

Rebase over https://reviews.llvm.org/D92084

I think I need some input on how best to handle this. I'm not particularly
happy with what I have right now, but after spending some time trying to do
better and not making much progress I thought I would ask if anyone else has a
better idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92690

Files:
  llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
  llvm/test/MC/AMDGPU/round-trip.s


Index: llvm/test/MC/AMDGPU/round-trip.s
===================================================================
--- /dev/null
+++ llvm/test/MC/AMDGPU/round-trip.s
@@ -0,0 +1,13 @@
+# RUN: llvm-mc -preserve-comments -triple amdgcn-amd-amdhsa %s >%t-1.s
+# RUN: llvm-mc -preserve-comments -triple amdgcn-amd-amdhsa %t-1.s >%t-2.s
+# RUN: diff %t-1.s %t-2.s
+
+# Test that AMDGPU assembly round-trips when run through MC; the first
+# transition from hand-written to "canonical" output may introduce some small
+# differences, so we don't include the initial input in the comparison.
+
+.text
+
+# The AMDGPU asm parser didn't consume the end of statement
+# consistently, which led to extra empty lines in the output.
+s_nop 0
Index: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
===================================================================
--- llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
+++ llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
@@ -4040,7 +4040,21 @@
     SMLoc ErrorLoc = IDLoc;
     if (ErrorInfo != ~0ULL) {
       if (ErrorInfo >= Operands.size()) {
-        return Error(getLoc(), "too few operands for instruction");
+        // FIXME: Unlike other targets, we try to use the SMLoc of the "end"
+        // of the instruction here. Everyone else does what we did before
+        // https://reviews.llvm.org/D92084 and points to IDLoc here.
+        //
+        // If this is left as getLoc() then we point to the beginning of the
+        // next (potential) statement, as we now parse the EndOfStatement token
+        // before returning from ParseInstruction.
+        //
+        // I don't see a clean way to recover the last position of the previous
+        // statement. `Operands.back()->getEndLoc() - 1` would also work below,
+        // but we don't properly record the end position for tokens currently
+        // and this seemed equivalent.
+        assert(getLexer().isAtStartOfStatement());
+        return Error(SMLoc::getFromPointer(getToken().getString().data() - 1),
+                     "too few operands for instruction");
       }
       ErrorLoc = ((AMDGPUOperand &)*Operands[ErrorInfo]).getStartLoc();
       if (ErrorLoc == SMLoc())
@@ -5020,9 +5034,11 @@
       while (!getLexer().is(AsmToken::EndOfStatement)) {
         Parser.Lex();
       }
+      Parser.Lex();
       return true;
     }
   }
+  Parser.Lex();
 
   return false;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D92690.310363.patch
Type: text/x-patch
Size: 2407 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201208/f9f903ad/attachment.bin>