This is an archive of the discontinued LLVM Phabricator instance.

[SDAG] Check fminnum for non zero operand.
ClosedPublic

Authored by samparker on Feb 3 2023, 2:38 AM.

Details

Summary

Following on from D143106...

Currently, in TargetLowering, if the target does not support fminnum, we lower to fminimum if neither operand could be a NaN. But this isn't quite correct because fminnum and fminimum treat +/-0 differently; so, we need to prove that one of the operands isn't a zero.

Diff Detail

Event Timeline

samparker created this revision.Feb 3 2023, 2:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2023, 2:38 AM
samparker requested review of this revision.Feb 3 2023, 2:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2023, 2:38 AM
spatel added inline comments.Feb 3 2023, 7:49 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7995

Could we also check hasNoSignedZeros() and allow this?

It would be good if you could copy the Arm tests so there are versions that can transform and versions that can't too.

samparker updated this revision to Diff 495076.Feb 6 2023, 4:13 AM
  • Added check for no signed zeros.
  • Fixed up vector support.
  • Added arm tests.
spatel added inline comments.Feb 6 2023, 5:49 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7980

Is this different than

auto *C = isConstOrConstSplatFP(N));
return C && C->isZero();
spatel added inline comments.Feb 6 2023, 5:51 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7980

Oops - invert that isZero() result (we're looking for non-zero constant).

samparker added inline comments.Feb 6 2023, 6:37 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7980

For splats, I think your suggestion would work, and that's what I originally tried to do... but it doesn't work for the BUILD_VECTOR case, where we need to check if any element is a zero.

spatel added inline comments.Feb 6 2023, 7:21 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7980

Ah, right. It looks like we do have the basic call already:
SelectionDAG::isKnownNeverZeroFloat().
...and it has a TODO comment about handling a vector constant, so just append that part there?

samparker added inline comments.Feb 6 2023, 7:24 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7980

Nice, will do.

samparker updated this revision to Diff 495132.Feb 6 2023, 7:35 AM

Modified, and now using, isKnownNeverZeroFloat.

spatel accepted this revision.Feb 6 2023, 7:47 AM

LGTM - would be best to pre-commit the new baseline tests, so we have that coverage in place even if the patch is reverted for some reason.

This revision is now accepted and ready to land.Feb 6 2023, 7:47 AM
This revision was landed with ongoing or failed builds.Feb 7 2023, 2:55 AM
This revision was automatically updated to reflect the committed changes.