This is an archive of the discontinued LLVM Phabricator instance.

RISC-V subtarget feature to disable floating-point division and square root instructions in codegen
Needs ReviewPublic

Authored by vb000 on Jun 19 2020, 2:29 PM.

Details

Reviewers
asb
Summary

This patch implements the subtarget feature no-fdiv to disable fdiv.[s/d] and fsqrt.[s/d] instructions when F or D extension is enabled:

  • The functionality is same as that of -mno-fdiv provided by GCC's RISC-V backend (https://gcc.gnu.org/onlinedocs/gcc/RISC-V-Options.html).
  • This throws and error when user enables this feature, but neither F nor D extension is enabled.
  • Implementation is done during ISel Lowering, using setOperationAction method on ISD::FDIV and ISD::FSQRT IR instructions.

Diff Detail

Event Timeline

vb000 created this revision.Jun 19 2020, 2:29 PM
vb000 updated this revision to Diff 272192.Jun 19 2020, 3:03 PM
vb000 edited the summary of this revision. (Show Details)Jun 19 2020, 3:56 PM
vb000 added a comment.Jun 30 2020, 4:44 PM

Hi,

Just a gentle reminder, if anyone's interested in reviewing this...

My question is: why do we want this additional complexity? F and D both require FMUL/FDIV to be implemented, so saying you support F (and D) but no FMUL/FDIV is a contradiction, no such implementation can possibly exist (it would instead be say RV32I plus a non-standard extension that looks like a subset of the floating-point one). If you want to optimise for area, don't include an FPU, and if you want speed, include one, but this seems like a strange halfway house. Just because GCC has an option for something doesn't mean it makes sense for us to copy. What is your actual use case for this? Are there RISC-V implementations in the wild that do this and we're missing out on being able to target them, or is this just a hypothetical?

vb000 added a comment.Jun 30 2020, 7:07 PM

Hi @jrtc27,

fdiv and fsqrt are rarely used instructions for many applications, but are beasts in terms of HW area consumption. While most of the F extension needs incremental additions to core FPU, fdiv and fsqrt need separate blocks of HW. Often this special purpose HW (whose sole purpose is to support two rarely used instructions) is not worth it in embedded applications. So, it's often fine to emulate those two instructions. So, there's definite value in having this option.

(Also, this is not implemented just because GCC has this. We need this option to use LLVM on an already taped-out chip. I'm just trying to make it easy to transition from GCC to LLVM for ourselves, as well for other who might find this useful.)

Hi @jrtc27,

fdiv and fsqrt are rarely used instructions for many applications, but are beasts in terms of HW area consumption. While most of the F extension needs incremental additions to core FPU, fdiv and fsqrt need separate blocks of HW. Often this special purpose HW (whose sole purpose is to support two rarely used instructions) is not worth it in embedded applications. So, it's often fine to emulate those two instructions. So, there's definite value in having this option.

(Also, this is not implemented just because GCC has this. We need this option to use LLVM on an already taped-out chip. I'm just trying to make it easy to transition from GCC to LLVM for ourselves, as well for other who might find this useful.)

Ok, but I object to saying F is enabled. It’s not. What’s implemented is a subset of it. So what you really want is a non-standard extension Xfnodivsqrt or something. If F is in my arch string I can always use fdiv and fsqrt, and those shouldn’t trap (provided mstatus.FS is suitably set).

arsenm added a subscriber: arsenm.Jul 23 2020, 7:03 PM

This is a subtarget feature and should not be referred to as an attribute

vb000 added a comment.Jul 23 2020, 7:41 PM

Hi @jrtc27,

fdiv and fsqrt are rarely used instructions for many applications, but are beasts in terms of HW area consumption. While most of the F extension needs incremental additions to core FPU, fdiv and fsqrt need separate blocks of HW. Often this special purpose HW (whose sole purpose is to support two rarely used instructions) is not worth it in embedded applications. So, it's often fine to emulate those two instructions. So, there's definite value in having this option.

(Also, this is not implemented just because GCC has this. We need this option to use LLVM on an already taped-out chip. I'm just trying to make it easy to transition from GCC to LLVM for ourselves, as well for other who might find this useful.)

Ok, but I object to saying F is enabled. It’s not. What’s implemented is a subset of it. So what you really want is a non-standard extension Xfnodivsqrt or something. If F is in my arch string I can always use fdiv and fsqrt, and those shouldn’t trap (provided mstatus.FS is suitably set).

I agree with this. But this a practically useful feature, especially for an open source ISA like RISC-V, because different organizations, groups or individuals with different budgets develops chips for one standard ISA. I hope these arguments convinces you with the usefulness of the feature. If usefulness of this feature is agreed upon, the remaining discussion would be about downsides of having this feature.

One downside of having this feature you mentioned is the divergence from the spec of 'F' standard extension. The only stakeholders that would be affected by the divergence from the spec are the very people that explicitly enabled this feature by passing the flag to llc. Since they know what to expect when they enable this feature and that the reasons are very much practical as I explained in my past comment, this doesn't seem to be an actual downside.

vb000 added a comment.Jul 23 2020, 7:45 PM

This is a subtarget feature and should not be referred to as an attribute

Okay. Subtarget features are enabled by passing -mattr=<> flag. By attribute, I was referring to this. We can refer this to as subtarget feature if that's the standard term.

vb000 retitled this revision from RISC-V machine attribute to disable floating-point division and square root instructions in codegen to RISC-V subtarget feature to disable floating-point division and square root instructions in codegen.Jul 23 2020, 7:46 PM
vb000 edited the summary of this revision. (Show Details)