This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support strict FP conversion operations.
ClosedPublic

Authored by craig.topper on Dec 18 2021, 1:29 PM.

Details

Summary

This adds support for strict conversions between fp types and between
integer and fp.

Diff Detail

Unit TestsFailed

Event Timeline

craig.topper created this revision.Dec 18 2021, 1:29 PM
craig.topper requested review of this revision.Dec 18 2021, 1:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 18 2021, 1:29 PM
Herald added a subscriber: MaskRay. · View Herald Transcript

Remove some stale test comments copied from non-strict tests.

Is it out of scope of this patch to check the rounding mode metadata argument? I've checked that changing the r.m. in a test case of e.g. fptrunc doesn't do anything, it just keeps emitting an fcvt with dynamic rounding. It's probably a good idea to add a short note describing the status/limitations of that to the tests / code / commit message, as appropriate.

Nit: is there any reason for not using a hard-float ABI in the tests? It seems like we could cut down on the loads/conversion noise in the tests. This probably also applies to D115680.

Is it out of scope of this patch to check the rounding mode metadata argument? I've checked that changing the r.m. in a test case of e.g. fptrunc doesn't do anything, it just keeps emitting an fcvt with dynamic rounding. It's probably a good idea to add a short note describing the status/limitations of that to the tests / code / commit message, as appropriate.

Rounding mode doesn't make it through SelectionDAGBuilder so isel can't use it. I think the metadata is intended to convey the compiler's knowledge about the most recent call to fesetround for constant folding purposes. (Though I don't think that information is captured at all today.) It is not intended to select static rounding mode. The dynamic mode was already updated so using a static instruction wouldn't do much unless the compiler was able to optimize out the dynamic change if all operations were able to use static rounding mode instruction.

I can add comments for this.

Nit: is there any reason for not using a hard-float ABI in the tests? It seems like we could cut down on the loads/conversion noise in the tests. This probably also applies to D115680.

Command lines and test case names came from the equivalent test files without "-strict". I only added the -disable-strictnode-mutation to make sure isel would fail if STRICT nodes were not handled. I diffed the files during patch development to make sure the same code was produced for the equivalent test without constrained intrinsics.

Command lines and test case names came from the equivalent test files without "-strict". I only added the -disable-strictnode-mutation to make sure isel would fail if STRICT nodes were not handled.

At first glance it looks like the non-strict files would also benefit from that ABI tweak, so both could be changed, but it's not very important.

I diffed the files during patch development to make sure the same code was produced for the equivalent test without constrained intrinsic.

I wish we could keep that testable with FileCheck but I suppose there's no good way to do that... :/

luismarques accepted this revision.Dec 22 2021, 7:30 AM

LGTM.

llvm/lib/Target/RISCV/RISCVInstrInfoF.td
22–25

I missed these in D106346 :)

This revision is now accepted and ready to land.Dec 22 2021, 7:30 AM
This revision was landed with ongoing or failed builds.Dec 23 2021, 7:41 AM
This revision was automatically updated to reflect the committed changes.