On Tue, Jun 13, 2023 at 6:03 PM Roger Sayle <ro...@nextmovesoftware.com> wrote:
>
>
> This patch is the next instalment in a set of backend patches around
> improvements to ptest/vptest. A previous patch optimized the sequence
> t=pand(x,y); ptestz(t,t) into the equivalent ptestz(x,y), using the
> property that ZF is set to (X&Y) == 0. This patch performs a similar
> transformation, converting t=pandn(x,y); ptestz(t,t) into the (almost)
> equivalent ptestc(y,x), using the property that the CF flags is set to
> (~X&Y) == 0. The tricky bit is that this sets the CF flag instead of
> the ZF flag, so we can only perform this transformation when we can
> also convert the flags' consumer, as well as the producer.
>
> For the test case:
>
> int foo (__m128i x, __m128i y)
> {
> __m128i a = x & ~y;
> return __builtin_ia32_ptestz128 (a, a);
> }
>
> With -O2 -msse4.1 we previously generated:
>
> foo: pandn %xmm0, %xmm1
> xorl %eax, %eax
> ptest %xmm1, %xmm1
> sete %al
> ret
>
> with this patch we now generate:
>
> foo: xorl %eax, %eax
> ptest %xmm0, %xmm1
> setc %al
> ret
>
> At the same time, this patch also provides alternative fixes for
> PR target/109973 and PR target/110118, by recognizing that ptestc(x,x)
> always sets the carry flag (X&~X is always zero). This is achieved
> both by recognizing the special case in ix86_expand_sse_ptest and with
> a splitter to convert an eligible ptest into an stc.
>
> The next piece is, of course, STV of "if (x & ~y)..."
>
> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> and make -k check, both with and without --target_board=unix{-m32}
> with no new failures. Ok for mainline?
>
> 2023-06-13 Roger Sayle <ro...@nextmovesoftware.com>
>
> gcc/ChangeLog
> * config/i386/i386-expand.cc (ix86_expand_sse_ptest): Recognize
> expansion of ptestc with equal operands as returning const1_rtx.
> * config/i386/i386.cc (ix86_rtx_costs): Provide accurate cost
> estimates of UNSPEC_PTEST, where the ptest performs the PAND
> or PAND of its operands.
> * config/i386/sse.md (define_split): Transform CCCmode UNSPEC_PTEST
> of reg_equal_p operands into an x86_stc instruction.
> (define_split): Split pandn/ptestz/setne into ptestc/setnc.
> (define_split): Split pandn/ptestz/sete into ptestc/setc.
> (define_split): Split pandn/ptestz/je into ptestc/jc.
> (define_split): Split pandn/ptestz/jne into ptestc/jnc.
>
> gcc/testsuite/ChangeLog
> * gcc.target/i386/avx-vptest-4.c: New test case.
> * gcc.target/i386/avx-vptest-5.c: Likewise.
> * gcc.target/i386/avx-vptest-6.c: Likewise.
> * gcc.target/i386/pr109973-1.c: Update test case.
> * gcc.target/i386/pr109973-2.c: Likewise.
> * gcc.target/i386/sse4_1-ptest-4.c: New test case.
> * gcc.target/i386/sse4_1-ptest-5.c: Likewise.
> * gcc.target/i386/sse4_1-ptest-6.c: Likewise.
>
>
> Thanks in advance,
> Roger
+ /* ptest reg, reg sets the carry flag. */
+ if (comparison == LTU
+ && (d->code == IX86_BUILTIN_PTESTC
+ || d->code == IX86_BUILTIN_PTESTC256)
+ && rtx_equal_p (op0, op1))
+ return const1_rtx;
In this function, a RTX that sets a target reg should be emitted, and
a target register returned. I don't think the above code is correct.
+;; pandn/ptestz/setne -> ptestc/setnc
+(define_split
+ [(set (match_operand:QI 0 "register_operand")
+ (ne:QI
Please note that setcc is a bit tricky on x86. You can actually set a
register in QI/HI/SI/DImode, and post-reload splitters will do the
correct extension (see the patterns in i386.md, after "For all sCOND
expanders ..." comment). But you have to account for all these modes
in the pre-reload splitter. Maybe you should use the
"int248_register_operand" predicate to avoid pattern explosion.
+ (unspec:CCZ [
+ (and:V_AVX (not:V_AVX (match_operand:V_AVX 1 "register_operand"))
+ (match_operand:V_AVX 2 "register_operand"))
+ (and:V_AVX (not:V_AVX (match_dup 1)) (match_dup 2))]
+ UNSPEC_PTEST)
+ (const_int 0)))]
+ "TARGET_SSE4_1"
+ [(set (reg:CCC FLAGS_REG)
+ (unspec:CCC [(match_dup 1) (match_dup 2)] UNSPEC_PTEST))
+ (set (strict_low_part (subreg:QI (match_dup 0) 0))
+ (geu:QI (reg:CCC FLAGS_REG) (const_int 0)))])
No need to set strict_low_part, just set a register with EQ/NE of
CCCmode and post-reload splitters will do their magic. Please also
note that you emit a QI subreg of a QI register here, which doesn't
seem right.
+
+;; Changing the CCmode of FLAGS_REG requires updating both def and use.
Does the above comment also apply to the above pattern?
+;; pandn/ptestz/sete -> ptestc/setc
+(define_split
+ [(set (strict_low_part (subreg:QI (match_operand:SI 0 "register_operand") 0))
+ (eq:QI
+ (unspec:CCZ [
+ (and:V_AVX (not:V_AVX (match_operand:V_AVX 1 "register_operand"))
+ (match_operand:V_AVX 2 "register_operand"))
+ (and:V_AVX (not:V_AVX (match_dup 1)) (match_dup 2))]
+ UNSPEC_PTEST)
+ (const_int 0)))]
+ "TARGET_SSE4_1"
+ [(set (reg:CCC FLAGS_REG)
+ (unspec:CCC [(match_dup 1) (match_dup 2)] UNSPEC_PTEST))
+ (set (strict_low_part (subreg:QI (match_dup 0) 0))
+ (ltu:QI (reg:CCC FLAGS_REG) (const_int 0)))])
These two patterns can be merged into one using the (somehow
unfortunately named) "bt_comparison_operator" (a new name is much
welcome...) operator predicate. The setc/setnc can easily be emitted
using eq/ne:QI (reg:CCC FLAGS_REG) (const_int 0).
+;; pandn/ptestz/je -> ptestc/jc
+(define_split
+ [(set (pc)
+ (if_then_else
+ (ne
+ (unspec:CCZ [
+ (and:V_AVX
+ (not:V_AVX (match_operand:V_AVX 1 "register_operand"))
+ (match_operand:V_AVX 2 "register_operand"))
+ (and:V_AVX (not:V_AVX (match_dup 1)) (match_dup 2))]
+ UNSPEC_PTEST)
+ (const_int 0))
+ (match_operand 0)
+ (pc)))]
+ "TARGET_SSE4_1"
+ [(set (reg:CCC FLAGS_REG)
+ (unspec:CCC [(match_dup 1) (match_dup 2)] UNSPEC_PTEST))
+ (set (pc) (if_then_else (geu (reg:CCC FLAGS_REG) (const_int 0))
+ (match_dup 0)
+ (pc)))])
+
+;; pandn/ptestz/jne -> ptestc/jnc
+(define_split
+ [(set (pc)
+ (if_then_else
+ (eq
+ (unspec:CCZ [
+ (and:V_AVX
+ (not:V_AVX (match_operand:V_AVX 1 "register_operand"))
+ (match_operand:V_AVX 2 "register_operand"))
+ (and:V_AVX (not:V_AVX (match_dup 1)) (match_dup 2))]
+ UNSPEC_PTEST)
+ (const_int 0))
+ (match_operand 0)
+ (pc)))]
+ "TARGET_SSE4_1"
+ [(set (reg:CCC FLAGS_REG)
+ (unspec:CCC [(match_dup 1) (match_dup 2)] UNSPEC_PTEST))
+ (set (pc) (if_then_else (ltu (reg:CCC FLAGS_REG) (const_int 0))
+ (match_dup 0)
+ (pc)))])
Also the above two can be merged using "bt_comparison_operator"
operator predicate.
+/* { dg-final { scan-assembler "ptest" } } */
+/* { dg-final { scan-assembler "jn?c" } } */
+/* { dg-final { scan-assembler-not "pandn" } } */
+/* { dg-final { scan-assembler-not "jne" } } */
+/* { dg-final { scan-assembler-not "je" } } */
Please better use scan-assembler-times when checking asm of several
functions. Otherwise, there could only be only one ptest (out of four)
generated, and the test will still pass.
+/* { dg-final { scan-assembler "ptest" } } */
+/* { dg-final { scan-assembler "setnc" } } */
+/* { dg-final { scan-assembler-not "pandn" } } */
+/* { dg-final { scan-assembler-not "setne" } } */
Also use scan-assembler-times when two functions are checked, same
reason as above.
Uros.
