[PATCH] D39893: [RISCV] MC layer support for the standard RV32F instruction set extension

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:362
+
+  void addFRMArgOperands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && "Invalid number of operands!");
----------------
reames wrote:
> This interface is odd.  I see you have a corresponding set of other functions, but why have these live on the operand type?
This is the RenderMethod field of AsmOperandClass, and the tablegen'erated code will always call it on the operand type. It adds the current operand to the given MCInst.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:365
+    // isFRMArg has validated the operand, meaning this cast is safe.
+    auto SE = cast<MCSymbolRefExpr>(getImm());
+
----------------
reames wrote:
> 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.
To clarify, you're looking for something like this?


```
  // Returns the rounding mode represented by this RISCVOperand. Should only
  // be called after checking isFRMArg.
  RISCVFPRndMode::RoundingMode getRoundingMode() const {
    // isFRMArg has validated the operand, meaning this cast is safe.
    auto SE = cast<MCSymbolRefExpr>(getImm());
    RISCVFPRndMode::RoundingMode FRM =
        RISCVFPRndMode::stringToRoundingMode(SE->getSymbol().getName());
    assert(FRM != RISCVFPRndMode::Invalid && "Invalid rounding mode");
    return FRM;
  }

  void addFRMArgOperands(MCInst &Inst, unsigned N) const {
    assert(N == 1 && "Invalid number of operands!");
    Inst.addOperand(MCOperand::createImm(getRoundingMode()));
  }
```


================
Comment at: lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp:77
+  auto FRMArg =
+      static_cast<RISCVFPRndMode::RoundingMode>(MI->getOperand(OpNo).getImm());
+  O << RISCVFPRndMode::roundingModeToString(FRMArg);
----------------
reames wrote:
> This is another use of the getRoundingMode helper.  
> 
> i.e. O << RISCVFPRndMode::roundingModeToString(MI->getOperand(OpNo)->getRndMode());
Unfortunately that's not possible. addFRMArgOperands adds the rounding mode as an integer operand type to the MCInst. We no longer have access to the RISCVOperand.


================
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,
----------------
reames wrote:
> Renumbering enums always makes me nervous.  Didn't check, but does this break any binary compat?
I don't think this is exposed through the LLVM library interface, and can't see any other way it would be exposed but could be missing something. Either way, we're an experimental target still and aren't trying to support users across LLVM dot releases.


https://reviews.llvm.org/D39893