This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Fix and expand fmin/fmax reassociation fold.
ClosedPublic

Authored by dmgreen on Jun 19 2023, 12:02 AM.

Details

Summary

This call to reassociateReduction is used by both fminnum/fmaxnum and fminimum/fmaximum. In adding support for fminimum/fmaximum we appear to be fixing the use of an incorrect reduction type, which should have only applied to minnum/maxnum.

I also believe that it doesn't need nsz and reassoc to perform the reassociation. For float min/max it should always be valid.

Diff Detail

Event Timeline

dmgreen created this revision.Jun 19 2023, 12:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2023, 12:02 AM
dmgreen requested review of this revision.Jun 19 2023, 12:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2023, 12:02 AM
nikic accepted this revision.Jun 19 2023, 1:25 AM

LGTM

llvm/test/CodeGen/AArch64/double_reduct.ll
83

Can you please also add a test with reduce.fmaximum combined with maxnum (or similar) to show that it does not fold?

This revision is now accepted and ready to land.Jun 19 2023, 1:25 AM
anna added a comment.Jun 20 2023, 9:30 AM

LGTM w/ a comment. Thanks for catching this!

llvm/test/CodeGen/AArch64/double_reduct.ll
40

Since we no longer check any flags for doing the reassociation, can you pls add a test without the fast flag as well?

anna accepted this revision.Jun 20 2023, 9:31 AM

Thanks. I've updated the tests in 71ac2a8e23f1587ca6243378d663aedf10b46642. It removes the fast math flags from llvm.vector.reduce.fmin/max tests, and adds some tests with a combo of fminnum/fminimum. The SVE tests I have left out for the moment until D153288 goes in to enable some scalable-vector lowering.

This revision was landed with ongoing or failed builds.Jun 23 2023, 6:45 AM
This revision was automatically updated to reflect the committed changes.