On Fri, May 15, 2026 at 9:13 AM Jakub Jelinek <[email protected]> wrote: > > Hi! > > IEEE min/max are not commutative and in the pattern > (define_insn "ieee_<ieee_maxmin><mode>3<mask_name><round_saeonly_name>" > [(set (match_operand:VFH 0 "register_operand" "=x,v") > (unspec:VFH > [(match_operand:VFH 1 "register_operand" "0,v") > (match_operand:VFH 2 "<round_saeonly_nimm_predicate>" > "xBm,<round_saeonly_constraint>")] > IEEE_MAXMIN))] > the first operand is a register and only the second one is register/memory. > Now, the *minmax<mode>3_3 define_insn_and_split does > rtx tmp = force_reg (<MODE>mode, operands[3]); > rtvec v = gen_rtvec (2, tmp, operands[2]); > operands[5] = gen_rtx_UNSPEC (<MODE>mode, v, u); > where operands[3] is the const0_operand, so operands[2] can there be > a memory, but in the *minmax<mode>3_4 case > rtx tmp = force_reg (<MODE>mode, operands[3]); > rtvec v = gen_rtvec (2, operands[2], tmp); > operands[5] = gen_rtx_UNSPEC (<MODE>mode, v, u); > operands[2] goes into the operand which must be a REG, so it > is incorrect to split it into something that won't work. > Now, I've tried both disabling the define_insn_and_split and > the following patch, the former to the latter results in > movaps a, %xmm0 > pxor %xmm1, %xmm1 > - cmpltps %xmm0, %xmm1 > - andps %xmm1, %xmm0 > + maxps %xmm1, %xmm0 > movaps %xmm0, a > ret > on the testcase, so I think it is better to match it and force_reg > (it is a pre-reload splitter) than change "nonimmediate_operand" > to "register_operand" because it won't match in that case.
Yes, this is the correct strategy. Pre-reload splitter is intended for the combine pass, so if we want to combine an insn that allows a memory operand, we should also allow memory operand in the combined insn, even if this means manually forcing memory op to the register in the preparation statement. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk and > 16.2? > > 2026-05-15 Jakub Jelinek <[email protected]> > > PR target/125308 > * config/i386/sse.md (*minmax<mode>3_4): Force also > operands[2] into a REG. > > * gcc.target/i386/pr125308.c: New test. OK for mainline and backports. Thanks, Uros. > > --- gcc/config/i386/sse.md.jj 2026-04-30 10:18:13.816688874 +0200 > +++ gcc/config/i386/sse.md 2026-05-14 21:16:32.951097873 +0200 > @@ -3468,7 +3468,7 @@ (define_insn_and_split "*minmax<mode>3_4 > u = UNSPEC_IEEE_MAX; > > rtx tmp = force_reg (<MODE>mode, operands[3]); > - rtvec v = gen_rtvec (2, operands[2], tmp); > + rtvec v = gen_rtvec (2, force_reg (<MODE>mode, operands[2]), tmp); > operands[5] = gen_rtx_UNSPEC (<MODE>mode, v, u); > }) > > --- gcc/testsuite/gcc.target/i386/pr125308.c.jj 2026-05-14 21:22:29.293954240 > +0200 > +++ gcc/testsuite/gcc.target/i386/pr125308.c 2026-05-14 21:22:03.967390890 > +0200 > @@ -0,0 +1,12 @@ > +/* PR target/125308 */ > +/* { dg-do compile } */ > +/* { dg-options "-msse2 -O2 -ffloat-store" } */ > + > +float a[4]; > + > +void > +foo () > +{ > + for (int i = 0; i < 4; i++) > + a[i] = a[i] > 0 ? a[i] : 0; > +} > > Jakub >
