[RISCV] MC layer support for the standard RV32F instruction set extension
ClosedPublic

Authored by asb on Nov 10 2017, 3:17 AM.

Details

Summary

The most interesting part of this patch to review is probably the handling of rounding mode arguments. Sadly, the RISC-V assembler handles floating point rounding modes as a special "argument" when it would be more consistent to handle them like the atomics, opcode suffixes. This patch supports parsing this optional parameter, using InstAlias to allow parsing these floating point instructions when no rounding mode is specified.

The 'DYN' rounding mode mnemonic is accepted by gas but isn't documented in the released ISA manual. I submitted a PR so this will be fixed in the next ISA doc. I've also documented the rounding modes in the work in progress RISC-V assembly programmers manual.

Diff Detail

Repository
rL LLVM
asb created this revision.Nov 10 2017, 3:17 AM
asb updated this revision to Diff 122414.Nov 10 2017, 3:30 AM

Small fix in rv32f-invalid.s.

apazos added inline comments.Thu, Nov 16, 5:49 PM
lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
98 ↗(On Diff #122414)

extra { }

asb updated this revision to Diff 123763.Tue, Nov 21, 4:24 AM
asb marked an inline comment as done.

Rebase and remove extra {} around single-statement if block.

reames accepted this revision.Mon, Dec 4, 6:39 PM
reames added a subscriber: reames.

LGTM w/comments addressed before submit.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
362 ↗(On Diff #123763)

This interface is odd. I see you have a corresponding set of other functions, but why have these live on the operand type?

365 ↗(On Diff #123763)

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.

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp
77 ↗(On Diff #123763)

This is another use of the getRoundingMode helper.

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

lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
29 ↗(On Diff #123763)

Renumbering enums always makes me nervous. Didn't check, but does this break any binary compat?

This revision is now accepted and ready to land.Mon, Dec 4, 6:39 PM
asb added inline comments.Wed, Dec 6, 3:58 AM
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
362 ↗(On Diff #123763)

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.

365 ↗(On Diff #123763)

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()));
}
lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp
77 ↗(On Diff #123763)

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.

lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
29 ↗(On Diff #123763)

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.

This revision was automatically updated to reflect the committed changes.