[PATCH] D141984: [RISCV][MC] Add support for experimental zfa extension(FLI instruction not included)

[Jun Sha via Phabricator via llvm-commits]

joshua-arch1 marked 3 inline comments as done.
joshua-arch1 added a comment.

Ping.



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1859
 
+OperandMatchResultTy RISCVAsmParser::parseRTZArg(OperandVector &Operands) {
+  if (getLexer().isNot(AsmToken::Identifier)) {
----------------
craig.topper wrote:
> Can we reuse parseFRMArg and check the value in isRTZArg()? Add a `DiagnosticType` to `RTZArg` and diagnose in RISCVAsmParser::MatchAndEmitInstruction
I think if we reuse parseFRMArg and check the value in isRTZArg(), the code quantity will be larger, since we need to add extra logic in isRTZArg() like what you did in initial isFRMArg() implementation. 


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1859
 
+OperandMatchResultTy RISCVAsmParser::parseRTZArg(OperandVector &Operands) {
+  if (getLexer().isNot(AsmToken::Identifier)) {
----------------
joshua-arch1 wrote:
> craig.topper wrote:
> > Can we reuse parseFRMArg and check the value in isRTZArg()? Add a `DiagnosticType` to `RTZArg` and diagnose in RISCVAsmParser::MatchAndEmitInstruction
> I think if we reuse parseFRMArg and check the value in isRTZArg(), the code quantity will be larger, since we need to add extra logic in isRTZArg() like what you did in initial isFRMArg() implementation. 
Since we have moved FRM parsing to a custom operand parser, it's briefer and more consistent to check all types of FRM operands in operand parsers instead of in isFRMArg()/isRTZArg(). 


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1859
 
+OperandMatchResultTy RISCVAsmParser::parseRTZArg(OperandVector &Operands) {
+  if (getLexer().isNot(AsmToken::Identifier)) {
----------------
craig.topper wrote:
> joshua-arch1 wrote:
> > joshua-arch1 wrote:
> > > craig.topper wrote:
> > > > Can we reuse parseFRMArg and check the value in isRTZArg()? Add a `DiagnosticType` to `RTZArg` and diagnose in RISCVAsmParser::MatchAndEmitInstruction
> > > I think if we reuse parseFRMArg and check the value in isRTZArg(), the code quantity will be larger, since we need to add extra logic in isRTZArg() like what you did in initial isFRMArg() implementation. 
> > Since we have moved FRM parsing to a custom operand parser, it's briefer and more consistent to check all types of FRM operands in operand parsers instead of in isFRMArg()/isRTZArg(). 
> This only seems to have been partially done?
> This only seems to have been partially done?

Did you mean 'reuse parseFRMArg and check the value in isRTZArg()'? I didn't do that. You can check my reply to your last comment.


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

https://reviews.llvm.org/D141984