[PATCH] D39895: [RISCV] MC layer support for the standard RV32D instruction set extension

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/changes applied.



================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:427
+  RISCVOperand &Op = static_cast<RISCVOperand &>(AsmOp);
+  if (Op.isReg()) {
+    unsigned Reg = Op.getReg();
----------------
asb wrote:
> apazos wrote:
> > consider early exit, return  Match_InvalidOperand from here if Op not Reg
> I considered that, but I don't think it's really a net improvement. You still need a return Match_InvalidOperand at the end ocatch the case where if (IsRegFPR32 && Kidn == MCK_FPR64) is false. I also think it's a little cleaner to use early return consistently for success or failure if feasible (e.g. in the current code, we only use early return for Match_Success which means it's easy to trace the control flow for failure). Using early return for !Op.isReg() would look like this. I prefer the current code, but let me know if you feel otherwise!
> 
> ```
> unsigned RISCVAsmParser::validateTargetOperandClass(MCParsedAsmOperand &AsmOp,
>                                                     unsigned Kind) {
>   RISCVOperand &Op = static_cast<RISCVOperand &>(AsmOp);
>   if (!Op.isReg())
>     return Match_InvalidOperand;
> 
>   unsigned Reg = Op.getReg();
>   bool IsRegFPR32 =
>       RISCVMCRegisterClasses[RISCV::FPR32RegClassID].contains(Reg);
> 
>   // As the parser couldn't differentiate an FPR32 from an FPR64, coerce the
>   // register from FPR32 to FPR64 if necessary.
>   if (IsRegFPR32 && Kind == MCK_FPR64) {
>     Op.Reg.RegNum = convertFPR32ToFPR64(Reg);
>     return Match_Success;
>   }
>   return Match_InvalidOperand;
> }
> ```
The early return variant is cleaner, or at least, much more idiomatic and thus easier to read quickly.  :)


https://reviews.llvm.org/D39895