[PATCH] D39893: [RISCV] MC layer support for the standard RV32F 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/comments addressed before submit.



================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:362
+
+  void addFRMArgOperands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && "Invalid number of operands!");
----------------
This interface is odd.  I see you have a corresponding set of other functions, but why have these live on the operand type?


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:365
+    // isFRMArg has validated the operand, meaning this cast is safe.
+    auto SE = cast<MCSymbolRefExpr>(getImm());
+
----------------
This looks like the body of a getRoundingMode function.  Extract and use please.  Add the assert within it that the operand is a rounding mode.


================
Comment at: lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp:77
+  auto FRMArg =
+      static_cast<RISCVFPRndMode::RoundingMode>(MI->getOperand(OpNo).getImm());
+  O << RISCVFPRndMode::roundingModeToString(FRMArg);
----------------
This is another use of the getRoundingMode helper.  

i.e. O << RISCVFPRndMode::roundingModeToString(MI->getOperand(OpNo)->getRndMode());


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:29
   InstFormatR = 1,
-  InstFormatI = 2,
-  InstFormatS = 3,
-  InstFormatB = 4,
-  InstFormatU = 5,
-  InstFormatJ = 6,
-  InstFormatOther = 7,
+  InstFormatR4 = 2,
+  InstFormatI = 3,
----------------
Renumbering enums always makes me nervous.  Didn't check, but does this break any binary compat?


https://reviews.llvm.org/D39893