Page MenuHomePhabricator

[SVE] Only combine (fneg (fma)) => FNMLA with nsz
ClosedPublic

Authored by peterwaller-arm on Sep 9 2021, 9:43 AM.

Details

Summary

-(Za + Zm * Zn) != (-Za + Zm * (-Zn))
when the FMA produces a zero output (e.g. all zero inputs can produce -0
output)

Add a PatFrag to check presence of nsz on the fneg, add tests which
ensure the combine does not fire in the absense of nsz.

See https://reviews.llvm.org/D90901 for a similar discussion on X86.

Diff Detail

Event Timeline

craig.topper created this revision.Sep 9 2021, 9:43 AM
craig.topper requested review of this revision.Sep 9 2021, 9:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2021, 9:43 AM
dmgreen added inline comments.Sep 9 2021, 11:59 AM
llvm/test/CodeGen/AArch64/sve-fp-combine.ll
405–408

These are all fast...?

craig.topper added inline comments.Sep 9 2021, 12:04 PM
llvm/test/CodeGen/AArch64/sve-fp-combine.ll
405–408

That's true, but the patterns weren't checking that. Would you prefer I add a PatFrag to check the fast math flags and keep the patterns?

dmgreen added inline comments.Sep 9 2021, 12:49 PM
llvm/test/CodeGen/AArch64/sve-fp-combine.ll
405–408

It would seem prudent to not delete obviously useful patterns. Having non-fast tests would be useful too, if we don't have them already.

Same goes for D109523 but the tests might be reversed there, where we have non-fast tests, but no nsz tests.

Matt added a subscriber: Matt.Sep 16 2021, 1:22 PM
craig.topper planned changes to this revision.Oct 3 2021, 11:42 PM

Hi @craig.topper, are you actively working on this or would you like us to pick it up?

Hi @craig.topper, are you actively working on this or would you like us to pick it up?

I'm not actively working on it. Would you mind picking it up?

peterwaller-arm commandeered this revision.Nov 4 2021, 8:55 AM
peterwaller-arm edited reviewers, added: craig.topper; removed: peterwaller-arm.

Sure, we'll pick it up (apols for the delayed response). Thanks for raising it.

peterwaller-arm retitled this revision from [SVE] Remove isel patterns for (fneg (fma)) -> fnmla. to [SVE] Only combine (fneg (fma)) => FNMLA with nsz.
peterwaller-arm edited the summary of this revision. (Show Details)
  • Update commit message / title
  • Introduce PatFrag to check for nsz on the fneg.
  • fmad tests no longer change, because -(fma) only arises as a consequence of nsz.

Couple of additional notes:

  • On main I landed a change to substitute fast for contract, which left the test output unmodified.
  • I had to introduce generic code to propagate fast math flags for unary op building, which as far as I can tell only affects the neg.
  • Likewise in LowerToPredicatedOp, it was necessary to propagate the fneg node's flags down to instructions.
dmgreen accepted this revision.Dec 9 2021, 1:56 AM

LGTM, with a small suggestion. Thanks for the updates.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
19112–19114

Can this just be return DAG.getNode(NewOp, DL, VT, Operands, Op->getFlags());?

This revision is now accepted and ready to land.Dec 9 2021, 1:56 AM
peterwaller-arm marked 2 inline comments as done.

Thanks for the improvement suggestion.

  • Pass Op->getFlags() via getNode.
peterwaller-arm marked an inline comment as done.Dec 9 2021, 3:10 AM
This revision was automatically updated to reflect the committed changes.