-Remove feq, fle, flt tests from *-arith.ll in favor of *-fcmp.ll which tests all predicates.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?
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.