x86: PR target/103611: Splitter for DST:DI = (HI:SI<<32)|LO:SI.
Commit Message
A common idiom is to create a DImode value from the "concat" of two SImode
values, using "(long long)hi << 32 | (long long)lo", where the operation
may be ior, xor or plus. On x86, with -m32, the high and low parts of
a DImode register are actually different SImode registers (typically %edx
and %eax) so ideally this idiom should reduce to two move instructions
(or optimally, just clever register allocation).
Unfortunately, GCC currently performs the IOR operation above on -m32,
and worse allocates DImode registers (split to SImode register pairs)
for both the zero extended HI and LO values.
Hence, for test1 from the new test case below:
typedef int __v4si __attribute__ ((__vector_size__ (16)));
long long test1(__v4si v) {
unsigned int loVal = (unsigned int)v[0];
unsigned int hiVal = (unsigned int)v[1];
return (long long)(loVal) | ((long long)(hiVal) << 32);
}
we currently generate (with -m32 -O2 -msse4.1):
test1: subl $28, %esp
pextrd $1, %xmm0, %eax
pmovzxdq %xmm0, %xmm1
movq %xmm1, 8(%esp)
movl %eax, %edx
movl 8(%esp), %eax
orl 12(%esp), %edx
addl $28, %esp
orb $0, %ah
ret
with this patch we now generate:
test1: pextrd $1, %xmm0, %edx
movd %xmm0, %eax
ret
The fix is to recognize and split the idiom (hi<<32)|zext(lo) prior
to register allocation on !TARGET_64BIT, simplifying this sequence to
"highpart(dst) = hi; lowpart(dst) = lo".
The one minor complication is that sse.md's define_insn for
*vec_extractv4si_0_zext_sse4 can sometimes interfere with this
optimization. It turns out that on !TARGET_64BIT, the zero_extend:DI
following vec_select:SI isn't free, and this insn gets split back
into multiple instructions during later passes, but too late to
be optimized away by this patch/reload. Hence the last hunk of
this patch is to restrict *vec_extractv4si_0_zext_sse4 to TARGET_64BIT.
Checking PR target/80286, where *vec_extractv4si_0_zext_sse4 was
first added, this seems reasonable (but this patch has been tested
both with and without this last change, if it's consider controversial).
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?
2021-12-13 Roger Sayle <roger@nextmovesoftware.com>
gcc/ChangeLog
PR target/103611
* config/i386/i386.md (any_or_plus): New code iterator.
(define_split): Split (HI<<32)|zext(LO) into piece-wise
move instructions on !TARGET_64BIT.
* config/i386/sse.md (*vec_extractv4si_0_zext_sse4):
Restrict to TARGET_64BIT.
gcc/testsuite/ChangeLog
PR target/103611
* gcc.target/i386/pr103611-2.c: New test case.
Thanks in advance,
Roger
--
Comments
"Roger Sayle" <roger@nextmovesoftware.com> writes:
> A common idiom is to create a DImode value from the "concat" of two SImode
> values, using "(long long)hi << 32 | (long long)lo", where the operation
> may be ior, xor or plus. On x86, with -m32, the high and low parts of
> a DImode register are actually different SImode registers (typically %edx
> and %eax) so ideally this idiom should reduce to two move instructions
> (or optimally, just clever register allocation).
>
> Unfortunately, GCC currently performs the IOR operation above on -m32,
> and worse allocates DImode registers (split to SImode register pairs)
> for both the zero extended HI and LO values.
>
> Hence, for test1 from the new test case below:
>
> typedef int __v4si __attribute__ ((__vector_size__ (16)));
> long long test1(__v4si v) {
> unsigned int loVal = (unsigned int)v[0];
> unsigned int hiVal = (unsigned int)v[1];
> return (long long)(loVal) | ((long long)(hiVal) << 32);
> }
>
> we currently generate (with -m32 -O2 -msse4.1):
>
> test1: subl $28, %esp
> pextrd $1, %xmm0, %eax
> pmovzxdq %xmm0, %xmm1
> movq %xmm1, 8(%esp)
> movl %eax, %edx
> movl 8(%esp), %eax
> orl 12(%esp), %edx
> addl $28, %esp
> orb $0, %ah
> ret
>
> with this patch we now generate:
>
> test1: pextrd $1, %xmm0, %edx
> movd %xmm0, %eax
> ret
>
> The fix is to recognize and split the idiom (hi<<32)|zext(lo) prior
> to register allocation on !TARGET_64BIT, simplifying this sequence to
> "highpart(dst) = hi; lowpart(dst) = lo".
>
> The one minor complication is that sse.md's define_insn for
> *vec_extractv4si_0_zext_sse4 can sometimes interfere with this
> optimization. It turns out that on !TARGET_64BIT, the zero_extend:DI
> following vec_select:SI isn't free, and this insn gets split back
> into multiple instructions during later passes, but too late to
> be optimized away by this patch/reload. Hence the last hunk of
> this patch is to restrict *vec_extractv4si_0_zext_sse4 to TARGET_64BIT.
> Checking PR target/80286, where *vec_extractv4si_0_zext_sse4 was
> first added, this seems reasonable (but this patch has been tested
> both with and without this last change, if it's consider controversial).
>
> 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?
To play devil's advocate: does this optimisation belong in
target-specific code? It feels pretty generic and could be keyed
off BITS_PER_WORD.
Thanks,
Richard
>
>
> 2021-12-13 Roger Sayle <roger@nextmovesoftware.com>
>
> gcc/ChangeLog
> PR target/103611
> * config/i386/i386.md (any_or_plus): New code iterator.
> (define_split): Split (HI<<32)|zext(LO) into piece-wise
> move instructions on !TARGET_64BIT.
> * config/i386/sse.md (*vec_extractv4si_0_zext_sse4):
> Restrict to TARGET_64BIT.
>
> gcc/testsuite/ChangeLog
> PR target/103611
> * gcc.target/i386/pr103611-2.c: New test case.
>
>
> Thanks in advance,
> Roger
> --
On Mon, Dec 13, 2021 at 3:10 PM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> A common idiom is to create a DImode value from the "concat" of two SImode
> values, using "(long long)hi << 32 | (long long)lo", where the operation
> may be ior, xor or plus. On x86, with -m32, the high and low parts of
> a DImode register are actually different SImode registers (typically %edx
> and %eax) so ideally this idiom should reduce to two move instructions
> (or optimally, just clever register allocation).
>
> Unfortunately, GCC currently performs the IOR operation above on -m32,
> and worse allocates DImode registers (split to SImode register pairs)
> for both the zero extended HI and LO values.
>
> Hence, for test1 from the new test case below:
>
> typedef int __v4si __attribute__ ((__vector_size__ (16)));
> long long test1(__v4si v) {
> unsigned int loVal = (unsigned int)v[0];
> unsigned int hiVal = (unsigned int)v[1];
> return (long long)(loVal) | ((long long)(hiVal) << 32);
> }
>
> we currently generate (with -m32 -O2 -msse4.1):
>
> test1: subl $28, %esp
> pextrd $1, %xmm0, %eax
> pmovzxdq %xmm0, %xmm1
> movq %xmm1, 8(%esp)
> movl %eax, %edx
> movl 8(%esp), %eax
> orl 12(%esp), %edx
> addl $28, %esp
> orb $0, %ah
> ret
>
> with this patch we now generate:
>
> test1: pextrd $1, %xmm0, %edx
> movd %xmm0, %eax
> ret
>
> The fix is to recognize and split the idiom (hi<<32)|zext(lo) prior
> to register allocation on !TARGET_64BIT, simplifying this sequence to
> "highpart(dst) = hi; lowpart(dst) = lo".
>
> The one minor complication is that sse.md's define_insn for
> *vec_extractv4si_0_zext_sse4 can sometimes interfere with this
> optimization. It turns out that on !TARGET_64BIT, the zero_extend:DI
> following vec_select:SI isn't free, and this insn gets split back
> into multiple instructions during later passes, but too late to
> be optimized away by this patch/reload. Hence the last hunk of
> this patch is to restrict *vec_extractv4si_0_zext_sse4 to TARGET_64BIT.
> Checking PR target/80286, where *vec_extractv4si_0_zext_sse4 was
> first added, this seems reasonable (but this patch has been tested
> both with and without this last change, if it's consider controversial).
>
> 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?
>
>
> 2021-12-13 Roger Sayle <roger@nextmovesoftware.com>
>
> gcc/ChangeLog
> PR target/103611
> * config/i386/i386.md (any_or_plus): New code iterator.
> (define_split): Split (HI<<32)|zext(LO) into piece-wise
> move instructions on !TARGET_64BIT.
> * config/i386/sse.md (*vec_extractv4si_0_zext_sse4):
> Restrict to TARGET_64BIT.
>
> gcc/testsuite/ChangeLog
> PR target/103611
> * gcc.target/i386/pr103611-2.c: New test case.
OK with *vec_extractv4si_0_zext_sse4 change but please also change isa
attribute to:
[(set_attr "isa" "*,*,avx512f")
Thanks,
Uros.
>
> Thanks in advance,
> Roger
> --
>
@@ -10620,6 +10620,38 @@
[(set_attr "isa" "*,nox64")
(set_attr "type" "alu")
(set_attr "mode" "QI")])
+
+;; Split DST = (HI<<32)|LO early to minimize register usage.
+(define_code_iterator any_or_plus [plus ior xor])
+(define_split
+ [(set (match_operand:DI 0 "register_operand")
+ (any_or_plus:DI
+ (ashift:DI (match_operand:DI 1 "register_operand")
+ (const_int 32))
+ (zero_extend:DI (match_operand:SI 2 "register_operand"))))]
+ "!TARGET_64BIT"
+ [(set (match_dup 3) (match_dup 4))
+ (set (match_dup 5) (match_dup 2))]
+{
+ operands[3] = gen_highpart (SImode, operands[0]);
+ operands[4] = gen_lowpart (SImode, operands[1]);
+ operands[5] = gen_lowpart (SImode, operands[0]);
+})
+
+(define_split
+ [(set (match_operand:DI 0 "register_operand")
+ (any_or_plus:DI
+ (zero_extend:DI (match_operand:SI 1 "register_operand"))
+ (ashift:DI (match_operand:DI 2 "register_operand")
+ (const_int 32))))]
+ "!TARGET_64BIT"
+ [(set (match_dup 3) (match_dup 4))
+ (set (match_dup 5) (match_dup 1))]
+{
+ operands[3] = gen_highpart (SImode, operands[0]);
+ operands[4] = gen_lowpart (SImode, operands[2]);
+ operands[5] = gen_lowpart (SImode, operands[0]);
+})
;; Negation instructions
@@ -18700,7 +18700,7 @@
(vec_select:SI
(match_operand:V4SI 1 "register_operand" "v,x,v")
(parallel [(const_int 0)]))))]
- "TARGET_SSE4_1"
+ "TARGET_64BIT && TARGET_SSE4_1"
"#"
[(set_attr "isa" "x64,*,avx512f")
(set (attr "preferred_for_speed")
new file mode 100644
@@ -0,0 +1,43 @@
+/* { dg-do compile } */
+/* { dg-options "-m32 -O2 -msse4" } */
+typedef int __v4si __attribute__ ((__vector_size__ (16)));
+
+long long test1(__v4si v) {
+ unsigned int loVal = (unsigned int)v[0];
+ unsigned int hiVal = (unsigned int)v[1];
+ return (long long)(loVal) | ((long long)(hiVal) << 32);
+}
+
+long long test2(__v4si v) {
+ unsigned int loVal = (unsigned int)v[2];
+ unsigned int hiVal = (unsigned int)v[3];
+ return (long long)(loVal) | ((long long)(hiVal) << 32);
+}
+
+long long test3(__v4si v) {
+ unsigned int loVal = (unsigned int)v[0];
+ unsigned int hiVal = (unsigned int)v[1];
+ return (long long)(loVal) ^ ((long long)(hiVal) << 32);
+}
+
+long long test4(__v4si v) {
+ unsigned int loVal = (unsigned int)v[2];
+ unsigned int hiVal = (unsigned int)v[3];
+ return (long long)(loVal) ^ ((long long)(hiVal) << 32);
+}
+
+long long test5(__v4si v) {
+ unsigned int loVal = (unsigned int)v[0];
+ unsigned int hiVal = (unsigned int)v[1];
+ return (long long)(loVal) + ((long long)(hiVal) << 32);
+}
+
+long long test6(__v4si v) {
+ unsigned int loVal = (unsigned int)v[2];
+ unsigned int hiVal = (unsigned int)v[3];
+ return (long long)(loVal) + ((long long)(hiVal) << 32);
+}
+
+/* { dg-final { scan-assembler-not "\tor" } } */
+/* { dg-final { scan-assembler-not "\txor" } } */
+/* { dg-final { scan-assembler-not "\tadd" } } */