This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add scalable vector fcmp ISel patterns
ClosedPublic

Authored by frasercrmck on Jan 7 2021, 9:39 AM.

Details

Summary

Original patch by @rogfer01.

All ordered comparisons except ONE are supported natively, and all
unordered comparisons except UNE are expanded into sequences involving
explicit NaN checks and mask arithmetic.

Additionally, we expand GT,OGT,GE,OGE to their swapped-operand versions, and
pattern-match those back to the "original", swapping operands once more. This
way we catch both operations and both "vf" and "fv" forms with fewer patterns.

Also add support for floating-point splat_vector, with an optimization for
splatting fpimm0.

Authored-by: Roger Ferrer Ibanez <rofirrim@gmail.com>
Co-Authored-by: Fraser Cormack <fraser@codeplay.com>

Diff Detail

Event Timeline

frasercrmck created this revision.Jan 7 2021, 9:39 AM
frasercrmck requested review of this revision.Jan 7 2021, 9:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2021, 9:39 AM
craig.topper added inline comments.Jan 7 2021, 3:12 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
349

Do we have nnan attribute tests to cover the conditions without O/U?

361

What if we expanded SETGT/SETOGT/SETGE/SETOGE and pattern matched VF from the LHS of SETLT/SETOLT/SETLE/SETOLE. Would that be a net reduction in patterns since we'll lose some of the VV patterns?

frasercrmck added inline comments.Jan 8 2021, 1:23 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
349

I'll look into that, cheers.

361

Possibly. Though I think I prefer it this way as it's more explicit than relying on the expansion to do what we want. Are you thinking more about wins in terms of the size of the generated tables?

  • add tests with no-nans attributes
frasercrmck marked an inline comment as done.Jan 8 2021, 3:22 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
349

Done. From what I can see the no-nans attribute is behaving as expected.

craig.topper added inline comments.Jan 8 2021, 9:53 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
361

Yeah I was trying to minimize the table size. I thought it would also make VF match easier since we would only need one pattern for each instruction except EQ/OEQ/NE/UNE. I think we already use the Expand for scalar nodes if I recall correctly.

craig.topper added inline comments.Jan 8 2021, 10:16 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
410

Should we use PseudoVMV_V_I_ for splat_vector fpimm0?

  • rebase on main
  • add optimization for fpimm0

expand o?(gt|ge) operations
support both vf and fv for all operations

frasercrmck marked 2 inline comments as done and an inline comment as not done.Jan 11 2021, 3:48 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
361

Good point. I have done that now: take a look. I think the table was slightly smaller (not sure how you typically measure that) in the interim, but it's a net-increase with the support for both VF and FV.

410

Yeah, good idea, though the fmv intrinsic splat uses PseudoVMV_V_X with X0 and I think it makes sense to make them do the same thing. Do you have a preference? The V_I feels better to me, intuitively.

frasercrmck edited the summary of this revision. (Show Details)Jan 11 2021, 3:50 AM
craig.topper accepted this revision.Jan 11 2021, 10:54 AM

LGTM

llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
361

There's a comment in the RISCVGenDAGISel.inc that prints that table size. I usually just search for "bytes".

This revision is now accepted and ready to land.Jan 11 2021, 10:54 AM
This revision was automatically updated to reflect the committed changes.
frasercrmck marked an inline comment as done.
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td