This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add patterns for scalable-vector fabs & fcopysign
ClosedPublic

Authored by frasercrmck on Feb 4 2021, 6:09 AM.

Details

Summary

The patterns mostly follow the scalar counterparts, save for some extra
optimizations to match the vector/scalar forms.

The patch adds a DAGCombine for ISD::FCOPYSIGN to try and reorder
ISD::FNEG around any ISD::FP_EXTEND or ISD::FP_TRUNC of the second
operand. This helps us achieve better codegen to match vfsgnjn.

Diff Detail

Event Timeline

frasercrmck created this revision.Feb 4 2021, 6:09 AM
frasercrmck requested review of this revision.Feb 4 2021, 6:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2021, 6:09 AM
  • add support for when the sign is trunc'd or extended from a different type
  • we could probably improve some of the code generation in this case (fneg isn't caught)
frasercrmck edited the summary of this revision. (Show Details)Feb 4 2021, 7:00 AM
frasercrmck edited the summary of this revision. (Show Details)
  • comment VFCOPYSIGN and minor fixups
craig.topper added inline comments.Feb 4 2021, 9:19 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2006

DAGCombien->DAGCombine

2007

Should this use the VFSGNJX name?

frasercrmck marked 2 inline comments as done.
  • fix typo
  • change name of custom node
frasercrmck added inline comments.Feb 5 2021, 2:55 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2006

Cheers

2007

Yeah that makes sense.

llvm/test/CodeGen/RISCV/rvv/vfcopysign-sdnode.ll
359

If I'm not mistaken, we should be able to improve the codegen of this example by reordering the fptrunc and fneg and folding the copysign+fneg to vfsgnjn?

  • rebase on main
  • rename RISCVISD::VFSGNJX to RISCVISD::VFSGNJ
  • try and propagate FNEG around extend/round to better match vfsgnjn
This revision is now accepted and ready to land.Feb 9 2021, 11:09 AM
  • rebase on main
  • rework codegen strategy to accomodate changes in FCOPYSIGN DAGCombine
frasercrmck edited the summary of this revision. (Show Details)Feb 10 2021, 7:55 AM

@craig.topper could you please take another look? After rebasing on D96037 the need for the custom node has gone, and the FNEG optimization was moved into a DAGCombine. The codegen for the tests didn't change.

craig.topper added inline comments.Feb 12 2021, 3:16 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2976

Should we avoid doing this if the extend/round is used elsewhere so we don't end up with extends/rounds of both the original and negated value?

frasercrmck edited the summary of this revision. (Show Details)
  • rebase
  • avoid fcopysign combine when the extend/round has > 1 use
frasercrmck marked an inline comment as done.Feb 15 2021, 4:02 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2976

Yeah probably because extends/rounds are typically more expensive than fneg. Will update.

This revision was automatically updated to reflect the committed changes.
frasercrmck marked an inline comment as done.