This is an archive of the discontinued LLVM Phabricator instance.

[X86] Improve handling on zero constant for fminimum/fmaximum lowering
ClosedPublic

Authored by skatkov on May 9 2023, 11:40 PM.

Details

Summary

If we know that zero constant operand is already in the right place we do not need
to re-order anything.

Diff Detail

Event Timeline

skatkov created this revision.May 9 2023, 11:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2023, 11:40 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
skatkov requested review of this revision.May 9 2023, 11:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2023, 11:40 PM
e-kud added inline comments.May 10 2023, 6:14 AM
llvm/test/CodeGen/X86/fminimum-fmaximum.ll
1167

I think this test shows that a predicate is not preferred zero should be more generic than is opposite zero (in context of constant vectors).

goldstein.w.n added inline comments.May 10 2023, 10:57 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
30312

nit: can you use a different variable name than PreferredZero as the value in the lambda doesn't always match whats in the outerscope.

skatkov added inline comments.May 10 2023, 9:05 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
30312

Agreed. Thanks, Will use just Zero.

llvm/test/CodeGen/X86/fminimum-fmaximum.ll
1167

I will modify test to use constant 0.f and -0.f to cover both not a preferred zero and not an opposite zero.
The intention of the test is to check that even if any of operands is zero we need all of them zero of the same sign.

skatkov updated this revision to Diff 521209.May 10 2023, 10:37 PM
e-kud added inline comments.May 11 2023, 5:15 AM
llvm/test/CodeGen/X86/fminimum-fmaximum.ll
1167

Sorry, my intentions were unclear. I mean in case of min(%x, <0, 5>) we need to generate a single instruction min(<0, 5>, %x). But we generate all kinds of checks instead. This happens because we special case only <0, 0, ...> for min. Instead we can check something like "a constant vector consisted of not preferred zero" this will cover <0, 0, ...> as well as <0, 5>.

skatkov added inline comments.May 11 2023, 8:11 PM
llvm/test/CodeGen/X86/fminimum-fmaximum.ll
1167

Got it. So you actually want another improvement. For vector we actually need preferred/opposite zero or not zero actually.
In this sense case with -0.0, 0.0 actually also makes sense.

So I need one more test case and improvement in the code.

e-kud accepted this revision.May 12 2023, 8:44 AM

Great! Thanks. LGTM.

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

It seems a little bit confusing for me that we generally do different things for vectors and scalars. Consider inputs 5. and <5., 5.>: for the first we return false, for the second -- true. But I can't think of better naming explaining this behavior difference, or splitting into different lambdas.

In fact, there is no much difference whether we return true or false because non zero cases are covered by isKnownNeverZeroFloat calls.

This revision is now accepted and ready to land.May 12 2023, 8:44 AM