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

[Lu Weining via Phabricator via llvm-commits]

SixWeining added inline comments.


================
Comment at: llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp:147
+  bool isUImm3() const { return isUImm<3>(); }
+  bool isUImm5() const { return isUImm<5>(); }
+  bool isUImm6() const { return isUImm<6>(); }
----------------
MaskRay wrote:
> These functions are unused. Usually it's better to add them in the patches that need them.
> 
> If the function is so simple, perhaps just use `isUImm<3>`. It just requires 2 more characters.
These functions are used by tablegen-ed code in `LoongArchGenAsmMatcher.inc`.


================
Comment at: llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp:228
+    assert(Expr && "Expr shouldn't be null!");
+    if (auto CE = dyn_cast<MCConstantExpr>(Expr))
+      Inst.addOperand(MCOperand::createImm(CE->getValue()));
----------------
MaskRay wrote:
> delete assert
> 
> dyn_cast asserts non-null
OK. Thanks!


================
Comment at: llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp:311
+/// Looks at a token type and creates the relevant operand from this
+/// information, adding to Operands. If operand was parsed, returns false, else
+/// true.
----------------
MaskRay wrote:
> `If operand was parsed, returns false, else ...` => Return true upon an error.
OK.


================
Comment at: llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp:316
+  // Attempt to parse token as a register.
+  if (parseRegister(Operands) == MatchOperand_Success)
+    return false;
----------------
MaskRay wrote:
> The code self explains.
> ```
> if (parseRegister(Operands) == MatchOperand_Success || parseImmediate(Operands) == MatchOperand_Success)
>   return false;
> ```
OK. Thanks.


================
Comment at: llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp:336
+  if (getLexer().is(AsmToken::EndOfStatement))
+    return false;
+
----------------
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.


================
Comment at: llvm/test/MC/LoongArch/Directives/cfi-valid.s:9
+# CHECK: .cfi_offset 22, -8
+.cfi_offset 22, -8
+# CHECK: .cfi_endproc
----------------
xen0n wrote:
> Maybe add checks for other registers as well?
OK. Thanks, let me add all others.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120476