[PATCH] D120476: [LoongArch] Add basic support to AsmParser

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

The file hierarchy `llvm/test/MC/LoongArch/ISA/Basic/Integer/arith.s` looks too long to me.

Do `ISA` and `Basic` in the path components improve clarity? If not, consider `llvm/test/MC/LoongArch/Integer/arith.s`
Note that for `test/MC/$target/`, we expect that most tests are ISA related...



================
Comment at: llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp:338
+  // Parse until end of statement, consuming commas between operands.
+  while (getLexer().is(AsmToken::Comma)) {
+    getLexer().Lex();
----------------
ditto, parseOptionalToken


================
Comment at: llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp:344
+
+  if (getLexer().isNot(AsmToken::EndOfStatement)) {
+    SMLoc Loc = getLexer().getLoc();
----------------
ditto. switch `then` and `else`, and use parseOptionalToken


================
Comment at: llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp:430
+  case Match_InvalidUImm2:
+    return generateImmOutOfRangeError(Operands, ErrorInfo, /*Lower=*/0,
+                                      /*Upper=*/(1 << 2) - 1);
----------------
Do you have negative tests for the invalid immediate Upper+1? There should be one for each immediate form.


================
Comment at: llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp:336
+  if (getLexer().is(AsmToken::EndOfStatement))
+    return false;
+
----------------
SixWeining wrote:
> MaskRay wrote:
> > This should be consumed. Not consuming it may cause some diagnostic line:column glitch, but I haven't tested.
> > 
> > `parseOptionalToken` can consume the token.
> Yes. Sorry I missed. Thanks.
You can use `parseOptionalToken` :) 

The comment hasn't been marked done.


================
Comment at: llvm/test/MC/LoongArch/Directives/data.s:1
+.ifndef ERR
+
----------------
Nit: 

```
# RUN: llvm-mc --triple=loongarch32 %s | FileCheck %s
# RUN: llvm-mc --triple=loongarch64 %s | FileCheck %s
# RUN: not llvm-mc --triple=loongarch32 --defsym=ERR=1 %s 2>&1 \
# RUN:     | FileCheck %s --check-prefix=CHECK-ERR
# RUN: not llvm-mc --triple=loongarch64 --defsym=ERR=1 %s 2>&1 \
# RUN:     | FileCheck %s --check-prefix=CHECK-ERR

positive tests ...

.ifdef ERR
negative tests ...
.endif
```


================
Comment at: llvm/test/MC/LoongArch/ISA/Basic/Integer/misc.s:6
+# RUN: llvm-mc %s --triple=loongarch64 -show-encoding --defsym=LA64=1 \
+# RUN:     | FileCheck --check-prefixes=CHECK32-ASM,CHECK64-ASM %s
+
----------------
`CHECK32-ASM` may be confusing when it actually apples to both 32-bit and 64-bit.

Perhaps `CHECK-ASM`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120476