This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add support for the various RISC-V FMA instruction variants
ClosedPublic

Authored by luismarques on Nov 7 2018, 6:13 AM.

Details

Summary

Adds support for the various RISC-V FMA instructions (fmadd, fmsub, fnmsub, fnmadd).

The criteria for choosing whether a fused add or subtract is used, as well as whether the product is negated or not, is whether some of the arguments to the llvm.fma.* intrinsic are negated or not. In the tests, extraneous fadd instructions were added to avoid the negation being performed using a xor trick, which prevented the proper FMA forms from being selected and thus tested.

The FMA instruction patterns might seem incorrect (e.g., fnmadd: -rs1 * rs2 - rs3), but they should be correct. The misleading names were inherited from MIPS, where the negation happens after computing the sum.

The llvm.fmuladd.* intrinsics still do not generate RISC-V FMA instructions, as that depends on TargetLowering::isFMAFasterthanFMulAndFAdd.

Some comments in the test files about what type of instructions are there tested were updated, to better reflect the current content of those test files.

Diff Detail

Repository
rL LLVM

Event Timeline

luismarques created this revision.Nov 7 2018, 6:13 AM
asb requested changes to this revision.Nov 30 2018, 7:04 AM

Just some minor tweaks and I think this is good to go. In addition to the changes mentioned in inline comments, please:

  • add nounwind to all the test functions
  • Remove tail from the calls to llvm.fma as it doesn't impact the generated code (I have a preference to remove everything that doesn't impact the generated code).

Thanks!

Given that the names of the FMA instructions have been a source of great confusion, I sanity checked vs gcc:

$ cat fma.c 
float fmadd(float a, float b, float c) {
  return a*b+c;
}

float fmsub(float a, float b, float c) {
  return a*b-c;
}

float fnmsub(float a, float b, float c) {
  return -a*b+c;
}

float fnmadd(float a, float b, float c) {
  return -a*b-c;
}
$ ./riscv32-unknown-elf-gcc -march=rv32imf -mabi=ilp32f fma.c -S -O2 -o - | grep -v [[:space:]]\\.
fmadd:
	fmadd.s	fa0,fa0,fa1,fa2
	ret
fmsub:
	fmsub.s	fa0,fa0,fa1,fa2
	ret
fnmsub:
	fnmsub.s	fa0,fa0,fa1,fa2
	ret
fnmadd:
	fnmadd.s	fa0,fa0,fa1,fa2
	ret
test/CodeGen/RISCV/double-intrinsics.ll
200 ↗(On Diff #172934)

Could you keep a test for llvm.fma in this and float-intrinsics.ll? I know it's covered by {float,double}-arith.ll, but it keeps things simpler if these -intrinsics.ll tests contain _all_ float intrinsics.

202 ↗(On Diff #172934)

Should be fmuladd_f64

test/CodeGen/RISCV/float-intrinsics.ll
185 ↗(On Diff #172934)

Should be fmuladd_f32

This revision now requires changes to proceed.Nov 30 2018, 7:04 AM

Fixes the issues pointed out by Alex Bradbury.

Some of the existing tests on the touched test files were changed:

  • Added nounwind attribute;
  • Fixed use of tabs, replaced with two spaces.
This revision was not accepted when it landed; it landed in state Needs Review.Dec 13 2018, 2:53 AM
This revision was automatically updated to reflect the committed changes.
asb added a comment.Dec 13 2018, 2:56 AM

Forgot to change the status in Phabricator prior to commit, but I did re-review the updated change and it definitely LGTM!