Bug 88540 - Issues with vectorization of min/max operations
Summary: Issues with vectorization of min/max operations
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 8.2.0
: P3 normal
Target Milestone: 14.0
Assignee: Richard Biener
URL:
Keywords: missed-optimization
Depends on:
Blocks: vectorizer 94864
  Show dependency treegraph
 
Reported: 2018-12-18 16:32 UTC by Daniel Fruzynski
Modified: 2023-07-31 11:31 UTC (History)
6 users (show)

See Also:
Host:
Target: x86_64-*-*, i?86-*-*
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-12-19 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Fruzynski 2018-12-18 16:32:46 UTC
1st issue:

[code]
#define SIZE 2

void test(double* __restrict d1, double* __restrict d2, double* __restrict d3)
{
    for (int n = 0; n < SIZE; ++n)
    {
        d3[n] = d1[n] < d2[n] ? d1[n] : d2[n];
    }
}
[code]

When this is compiled with for SSE2, gcc produces non vectorized code:

[asm]
test(double*, double*, double*):
        vmovsd  xmm0, QWORD PTR [rdi]
        vminsd  xmm0, xmm0, QWORD PTR [rsi]
        vmovsd  QWORD PTR [rdx], xmm0
        vmovsd  xmm0, QWORD PTR [rdi+8]
        vminsd  xmm0, xmm0, QWORD PTR [rsi+8]
        vmovsd  QWORD PTR [rdx+8], xmm0
        ret
[/asm]

When SIZE is changed to 3 or greater, code gets vectorized properly. I thought that this may be some workaround for old CPU which was slower there, but this also happen when compiling with "-O3 -march=skylake". I also checked with SIZE 6, and got 1 AVX op and 2 scalar SSE ones. Looks that this is an off-by-one bug.

The same happen for code with other relational operators (>, <=, >=).

2nd issue: when compiling for AVX512, gcc does not use new instructions which use ZMM registers, it still generates code for YMM ones.
Comment 1 Richard Biener 2018-12-19 09:16:03 UTC
This is because without -ffast-math the completely unrolled loop isn't if-converted to MIN and thus basic-block vectorization fails.  With
loop vectorization we apply if-conversion:

  _1 = (long unsigned int) n_20;
  _2 = _1 * 8;
  _3 = d1_12(D) + _2;
  _4 = *_3;
  _5 = d2_13(D) + _2;
  _6 = *_5;
  iftmp.0_9 = _4 < _6 ? _4 : _6;
  _7 = d3_14(D) + _2;
  *_7 = iftmp.0_9;
  n_16 = n_20 + 1;

and vectorize it as

  vect_iftmp.7_43 = VEC_COND_EXPR <vect__4.3_39 < vect__6.6_42, vect__4.3_39, vect__6.6_42>;

ending up as

(insn 12 11 13 (set (reg:V2DF 98 [ vect_iftmp.7 ])
        (unspec:V2DF [
                (reg:V2DF 87 [ vect__4.3 ])
                (reg:V2DF 88 [ vect__6.6 ])
            ] UNSPEC_IEEE_MIN)) "t.c":7 -1
     (nil))

and exactly the same assembly as with -ffast-math.

So the issue is that we do not if-convert the MIN pattern to use
a COND_EXPR in phiopt [when the target has an IEEE MIN we can use].
Or, that basic-block vectorization does not perform if-conversion
on non-loop code.

You can workaround in your code with

#pragma GCC unroll 0
  for (int n = 0; n < SIZE; ++n)
    {
      d3[n] = d1[n] < d2[n] ? d1[n] : d2[n];
    }

keeping the loop and using loop vectorization.

Note the backend could implement the fmin/fmax optabs which allows
more optimizations.  Also minmax_replacement in phi-opt could make
use of the FMIN/FMAX IFNs when HONOR_NANS || HONOR_SIGNED_ZEROS
and the direct IFN is available.
Comment 2 Richard Biener 2018-12-19 09:17:31 UTC
aarch64 already implements fmin/fmax patterns so I wonder if the same issue replicates there and teaching phi-opt would help.
Comment 3 Daniel Fruzynski 2018-12-19 09:30:43 UTC
Looks that AARCH64 is also affected. This is output from gcc 8.2 for SIZE=2:

[asm]
test(double*, double*, double*):
        ldp     d1, d0, [x0]
        ldp     d3, d2, [x1]
        fcmpe   d1, d3
        fcsel   d1, d1, d3, mi
        fcmpe   d0, d2
        fcsel   d0, d0, d2, mi
        stp     d1, d0, [x2]
        ret
[/asm]

And this is for SIZE=4:

[asm]
test(double*, double*, double*):
        ldr     q5, [x0]
        ldr     q3, [x1]
        ldr     q4, [x0, 16]
        ldr     q2, [x1, 16]
        fcmgt   v1.2d, v3.2d, v5.2d
        fcmgt   v0.2d, v2.2d, v4.2d
        bsl     v1.16b, v5.16b, v3.16b
        bsl     v0.16b, v4.16b, v2.16b
        str     q1, [x2]
        str     q0, [x2, 16]
        ret
[/asm]
Comment 4 Richard Biener 2020-05-07 12:44:35 UTC
Mine.
Comment 5 Drea Pinski 2023-04-10 00:40:39 UTC
> Also minmax_replacement in phi-opt could make
> use of the FMIN/FMAX IFNs when HONOR_NANS || HONOR_SIGNED_ZEROS
> and the direct IFN is available.

You should just need to teach match.pd rather than minmax_replacement to do these days. I have patches that start to remove minmax_replacement even.
Comment 6 Hongtao.liu 2023-07-13 01:45:36 UTC
Note, x86 MINSS/MAXSS is not IEEE-conformant minimum and maximum operations, it always return the second operand when there's NAN, but for fmin/fmax_optab it should return the other operand.
Comment 7 Hongtao.liu 2023-07-13 01:54:52 UTC
(In reply to Hongtao.liu from comment #6)
> Note, x86 MINSS/MAXSS is not IEEE-conformant minimum and maximum operations,
> it always return the second operand when there's NAN, but for
> fmin/fmax_optab it should return the other operand.

But for this case, it shoud match MINSD d1[n], d2[n].
if d1[n] or d2[n] is NAN, it will always return d2[n](the second operand).
the conclusion also holds for sign-zeros, ieee take -0.0 equal as 0.0, which means -0.0 < 0.0 should be false, then the second operand d2[n] should be returned.
Comment 8 GCC Commits 2023-07-21 07:35:42 UTC
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:9f8f37f5490076b10436993fb90d18092a960922

commit r14-2699-g9f8f37f5490076b10436993fb90d18092a960922
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Jul 13 08:58:58 2023 +0200

    tree-optimization/88540 - FP x > y ? x : y if-conversion without -ffast-math
    
    The following makes sure that FP x > y ? x : y style max/min operations
    are if-converted at the GIMPLE level.  While we can neither match
    it to MAX_EXPR nor .FMAX as both have different semantics with IEEE
    than the ternary ?: operation we can make sure to maintain this form
    as a COND_EXPR so backends have the chance to match this to instructions
    their ISA offers.
    
    The patch does this in phiopt where we recognize min/max and instead
    of giving up when we have to honor NaNs we alter the generated code
    to a COND_EXPR.
    
    This resolves PR88540 and we can then SLP vectorize the min operation
    for its testcase.  It also resolves part of the regressions observed
    with the change matching bit-inserts of bit-field-refs to vec_perm.
    
    Expansion from a COND_EXPR rather than from compare-and-branch
    gcc.target/i386/pr54855-9.c by producing extra moves while the
    corresponding min/max operations are now already synthesized by
    RTL expansion, register selection isn't optimal.  This can be also
    provoked without this change by altering the operand order in the source.
    I have XFAILed that part of the test.
    
            PR tree-optimization/88540
            * tree-ssa-phiopt.cc (minmax_replacement): Do not give up
            with NaNs but handle the simple case by if-converting to a
            COND_EXPR.
    
            * gcc.target/i386/pr88540.c: New testcase.
            * gcc.target/i386/pr54855-9.c: XFAIL check for redundant moves.
            * gcc.target/i386/pr54855-12.c: Adjust.
            * gcc.target/i386/pr54855-13.c: Likewise.
            * gcc.target/i386/pr110170.c: Likewise.
            * gcc.dg/tree-ssa/split-path-12.c: Likewise.
Comment 9 Richard Biener 2023-07-31 11:31:27 UTC
Both issues are now fixed.