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

[Lu Weining via Phabricator via llvm-commits]

SixWeining marked 6 inline comments as done.
SixWeining added a comment.
Herald added a project: All.

In D120476#3352182 <https://reviews.llvm.org/D120476#3352182>, @MaskRay wrote:

> 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...

About the hierarchy, since `relocations` and `LASX/LSX/LVZ` would be introduced in future and `LASX/LSX` apply both to `Integer` and `FloatingPoint`, so my plan is

  LoongArch/
  ├── Directives
  ├── ISA
  │   ├── Basic
  │   │   ├── FloatingPoint
  │   │   ├── Integer
  │   │   ├── Privileged
  │   ├── LASX
  │   └── LSX
  │   └── LVZ
  └── Misc
  └── Relocations

How about this:

  LoongArch/
  ├── Basic
  │   ├── FloatingPoint
  │   ├── Integer
  │   └── Privileged
  ├── Directives
  ├── LASX
  ├── LSX
  ├── LVZ
  ├── Misc
  └── Relocations



================
Comment at: llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp:430
+  case Match_InvalidUImm2:
+    return generateImmOutOfRangeError(Operands, ErrorInfo, /*Lower=*/0,
+                                      /*Upper=*/(1 << 2) - 1);
----------------
MaskRay wrote:
> Do you have negative tests for the invalid immediate Upper+1? There should be one for each immediate form.
Yes, I have. They are in `test/MC/LoongArch/ISA/Basic/Integer/invalid.s`.


================
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
+
----------------
MaskRay wrote:
> `CHECK32-ASM` may be confusing when it actually apples to both 32-bit and 64-bit.
> 
> Perhaps `CHECK-ASM`
Thanks. Should be `CHECK-ASM`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120476