This is an archive of the discontinued LLVM Phabricator instance.

[X86] Avoid usage constant NaN for fminimum/fmaximum lowering
ClosedPublic

Authored by skatkov on May 3 2023, 12:54 AM.

Details

Summary

After applying FMIN/FMAX, if any of operands is NaN, the second operand will be the result.
So all we need is to check whether first operand is NaN and return it or result of FMIN/FMAX.

So we avoid usage of constant NaN in the lowering.

Additionally we can avoid handling NaN after FMIN/FMAX if we are sure that first operand is not NaN.

Diff Detail

Event Timeline

skatkov created this revision.May 3 2023, 12:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2023, 12:54 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
skatkov requested review of this revision.May 3 2023, 12:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2023, 12:54 AM
goldstein.w.n added inline comments.May 3 2023, 8:20 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
30275–30276

This doesn't need to be fwd declared anymore.

30278–30284

'Singed' -> 'Signed'

e-kud added a comment.May 3 2023, 6:39 PM

Good catch! Missed that we don't need to check Y on NaN because it is already a result of min therefore we can get rid of NaN loading.

llvm/lib/Target/X86/X86ISelLowering.cpp
30258

Since now we return one of NaNs. Could you please update the table as well?

30285

This is a little out of code-style to have an empty if. I don't see any of it in lib/Target/X86. Can we reorganize somehow to avoid it?

skatkov updated this revision to Diff 519339.May 3 2023, 7:42 PM
skatkov marked 4 inline comments as done.
skatkov added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
30285

instead of re-factoring, I decided to make it explicit that X and Y are not re-ordered. So block becomes non-empty.

e-kud accepted this revision.May 4 2023, 5:27 AM

LGTM. Glibc tests pass (I haven't checked correspondence of qnan/snan though).

This revision is now accepted and ready to land.May 4 2023, 5:27 AM
This revision was automatically updated to reflect the committed changes.
skatkov marked an inline comment as done.