This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Reduce duplicate FP test cases.
ClosedPublic

Authored by craig.topper on Nov 11 2021, 11:21 AM.

Details

Summary

-Remove feq, fle, flt tests from *-arith.ll in favor of *-fcmp.ll which tests all predicates.

Diff Detail

Event Timeline

craig.topper created this revision.Nov 11 2021, 11:21 AM
craig.topper requested review of this revision.Nov 11 2021, 11:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2021, 11:21 AM
Herald added a subscriber: MaskRay. · View Herald Transcript

Hm, I'd expect all the tests that call @llvm.foo to be in -instrinsics.ll given that's what they are; -arith.ll just isn't a great name, because all of this is basically arithmetic.

Hm, I'd expect all the tests that call @llvm.foo to be in -instrinsics.ll given that's what they are; -arith.ll just isn't a great name, because all of this is basically arithmetic.

Hm, I'd expect all the tests that call @llvm.foo to be in -instrinsics.ll given that's what they are; -arith.ll just isn't a great name, because all of this is basically arithmetic.

I forgot to mention that *-arith.ll has the tests for all the fma flavors. *-intrinsics.ll only has the basic test. The -arith.ll files also have this comment at the top "These tests are each targeted at a particular RISC-V FPU instruction. Most other files in this folder exercise LLVM IR instructions that don't directly match a RISC-V instruction.". Though that's not strictly true since conversion instructions are in different files already.

-Remove some things from -intrinsics.ll instead.
-Keep copysign and fabs in -arith.. so that all the sign injection tests are together.
-Kept FMA in arith because it had more complete coverage with negations. May
change FMA in another patch. We need negation coverage on fmuladd intrinsic too.
So maybe an fma file for fma and fmuladd makes sense?

craig.topper edited the summary of this revision. (Show Details)Nov 11 2021, 12:41 PM
asb added a comment.Nov 12 2021, 2:32 AM

To say a little more about naming and how it got this way (no argument that it's confusing / could be improved!):
While implementing the backend I came at FP support from two directions - firstly going through the ISA manual and ensuring patterns were in place demonstrating the generation of each instruction (the *-arith.ll files), and then going through the LLVM IR manual and checking other backends test files to ensure we have coverage of all intrinsics (the *-intrinsics.ll files) - some being directly lowered to instructions, others appropriately expanded. The duplication is inelegant, but I figured it may be confusing if trying to validate those files were complete to find them incomplete. Of course that's all an artifact of what I personally found useful when bringing the backend up, and needn't constrain what we want to do moving forwards. It does still make some logical sense in my mind to group together tests that are effectively written alongside the selectiondag patterns though.

I've got no objections to the changes proposed in the patch (and the suggestion to split out fma to separate file seems fine), but perhaps the header comment could also be updated or just removed.

Reducing scope to just compares. The fact that we have a logically named file
for fcmps makes this probably the least controversial part. Updated file header
to point out the other files.

craig.topper edited the summary of this revision. (Show Details)Nov 12 2021, 12:19 PM
asb accepted this revision.Dec 9 2021, 5:29 AM

Sorry, missed I hadn't come back on this. LGTM.

This revision is now accepted and ready to land.Dec 9 2021, 5:29 AM
This revision was landed with ongoing or failed builds.Dec 9 2021, 8:41 AM
This revision was automatically updated to reflect the committed changes.