This patch fixes the optimization regression that occurred on GCC 7 where
conversions from the various floating point types to small integers would at
times generate a store and a load.
For example, converting from double to unsigned char generated the following
code on GCC 6 for -mcpu=power8:
fctiwuz 1,1
mfvsrd 3,1
rlwinm 3,3,0,0xff
on GCC 7 and 8 it generates:
fctiwuz 0,1
mfvsrwz 9,0
stw 9,-16(1)
ori 2,2,0
lbz 3,-16(1)
The insns before register allocation are:
(insn 7 8 13 2 (set (subreg:SI (reg:QI 157) 0)
(unsigned_fix:SI (reg:SF 33)))
(insn 13 7 14 2 (set (reg/i:DI 3 3)
(zero_extend:DI (reg:QI 157))))
After reload, the insns are:
(insn 7 8 19 2 (set (reg:SI 32 0 [160])
(unsigned_fix:SI (reg:SF 33))))
(insn 19 7 18 2 (set (reg:SI 9 9 [160])
(reg:SI 32 0 [160])))
(insn 18 19 13 2 (set (mem/c:SI (plus:DI (reg/f:DI 1 1)
(const_int -16))
(reg:SI 9))))
(insn 13 18 14 2 (set (reg/i:DI 3 3)
(zero_extend:DI (mem/c:HI (plus:DI (reg/f:DI 1 1)
(const_int
-16))))))
ISA 3.0 (Power9) did not have this problem, because it already had a
fixuns_truncdfqi2 pattern, since QI/HImode values are allowed in vector
registers. Previous versions of the ISA did not allow QI/HImode into vector
registers, because there wasn't load or store byte/half-word operations.
I extended ISA 3.0 conversion patterns to handle ISA 2.07, using splitters to
move the 32-bit int parts back to the GPR to do sign/zero extension or stores.
I also moved the optimization to prevent the register allocator from doing a
direct move on ISA 3.0 to do an offsettable store via the GPR register to a
separate insn, like I had previously done for SImode. The rationale for this
is to prevent some places where the register allocator decided to do change a
store into a move (and then later store).
I have tested this patch on a little endian power8 system (64-bit) and a big
endian power8 system (both 32-bit and 64-bit executables). There were no
regressions in the test suite and the compiler bootstrapped fine. I added some
tests, and verified they ran in all 3 environments. Can I check this into the
trunk? Given this is a regression in GCC 7 as well, can I check the patch if
it applies cleanly into GCC 7 after a burn-in period.
[gcc]
2018-02-01 Michael Meissner <meiss...@linux.vnet.ibm.com>
PR target/84154
* config/rs6000/rs6000.md (fix_trunc<SFDF:mode><QHI:mode>2):
Convert from define_expand to be define_insn_and_split. Rework
float/double/_Float128 conversions to QI/HI/SImode to work with
both ISA 2.07 (power8) or ISA 3.0 (power9). Fix regression where
conversions to QI/HImode types did a store and then a load to
truncate the value. For conversions to VSX registers, don't split
the insn, instead emit the code directly.
(fixuns_trunc<SFDF:mode><QHI:mode>2): Likewise.
(fix_trunc<IEEE128:mode><QHI:mode>2): Likewise.
(fix_trunc<SFDF:mode><QHI:mode>2_internal): Delete, no longer
used.
(fixuns_trunc<SFDF:mode><QHI:mode>2_internal): Likewise.
(fix<uns>_<mode>_mem): Likewise.
(fix_trunc<SFDF:mode><QHI:mode>2_mem): On ISA 3.0, prevent the
register allocator from doing a direct move to the GPRs to do a
store, and instead use the ISA 3.0 store byte/half-word from
vector register instruction. For IEEE 128-bit floating point,
also optimize stores of 32-bit ints.
(fixuns_trunc<SFDF:mode><QHI:mode>2_mem): Likewise.
(fix_trunc<IEEE128:mode><QHSI:mode>2_mem): Likewise.
(fixuns_trunc<IEEE128:mode><QHSI:mode>2_mem): Likewise.
[gcc/testsuite]
2018-02-01 Michael Meissner <meiss...@linux.vnet.ibm.com>
PR target/84154
* gcc.target/powerpc/pr84154-1.c: New tests.
* gcc.target/powerpc/pr84154-2.c: Likewise.
* gcc.target/powerpc/pr84154-3.c: Likewise.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md (revision 257269)
+++ gcc/config/rs6000/rs6000.md (working copy)
@@ -5700,43 +5700,48 @@ (define_insn "*fix_trunc<mode>di2_fctidz
xscvdpsxds %x0,%x1"
[(set_attr "type" "fp")])
-(define_expand "fix_trunc<SFDF:mode><QHI:mode>2"
- [(parallel [(set (match_operand:<QHI:MODE> 0 "nonimmediate_operand")
- (fix:QHI (match_operand:SFDF 1 "gpc_reg_operand")))
- (clobber (match_scratch:DI 2))])]
- "TARGET_P9_VECTOR && TARGET_DIRECT_MOVE_64BIT"
+;; If have ISA 3.0, QI/HImode values can go in both VSX registers and GPR
+;; registers. If we have ISA 2.07, we don't allow QI/HImode values in the
+;; vector registers, so we need to do direct moves to the GPRs, but SImode
+;; values can go in VSX registers. Keeping the direct move part through
+;; register allocation prevents the register allocator from doing a direct of
+;; the SImode value to a GPR, and then a store/load.
+(define_insn_and_split "fix_trunc<SFDF:mode><QHI:mode>2"
+ [(set (match_operand:<QHI:MODE> 0 "gpc_reg_operand" "=wJ,wJwK,r")
+ (fix:QHI (match_operand:SFDF 1 "gpc_reg_operand" "wJ,wJwK,wa")))
+ (clobber (match_scratch:SI 2 "=X,X,wi"))]
+ "TARGET_DIRECT_MOVE"
+ "@
+ fctiwz %0,%1
+ xscvdpsxws %x0,%x1
+ #"
+ "&& reload_completed && int_reg_operand (operands[0], <QHI:MODE>mode)"
+ [(set (match_dup 2)
+ (fix:SI (match_dup 1)))
+ (set (match_dup 3)
+ (match_dup 2))]
{
- if (MEM_P (operands[0]))
- operands[0] = rs6000_address_for_fpconvert (operands[0]);
-})
+ operands[3] = gen_rtx_REG (SImode, REGNO (operands[0]));
+}
+ [(set_attr "length" "4,4,8")
+ (set_attr "type" "fp")])
-(define_insn_and_split "*fix_trunc<SFDF:mode><QHI:mode>2_internal"
- [(set (match_operand:<QHI:MODE> 0 "reg_or_indexed_operand" "=wIwJ,rZ")
- (fix:QHI
- (match_operand:SFDF 1 "gpc_reg_operand" "<SFDF:Fv>,<SFDF:Fv>")))
- (clobber (match_scratch:DI 2 "=X,wi"))]
- "TARGET_P9_VECTOR && TARGET_DIRECT_MOVE_64BIT"
+;; Keep the convert and store together through register allocation to prevent
+;; the register allocator from getting clever and doing a direct move to a GPR
+;; and then store for reg+offset stores.
+(define_insn_and_split "*fix_trunc<SFDF:mode><QHI:mode>2_mem"
+ [(set (match_operand:QHI 0 "memory_operand" "=Z")
+ (fix:QHI (match_operand:SFDF 1 "gpc_reg_operand" "wa")))
+ (clobber (match_scratch:SI 2 "=wa"))]
+ "TARGET_P9_VECTOR"
"#"
"&& reload_completed"
- [(const_int 0)]
+ [(set (match_dup 2)
+ (fix:SI (match_dup 1)))
+ (set (match_dup 0)
+ (match_dup 3))]
{
- rtx dest = operands[0];
- rtx src = operands[1];
-
- if (vsx_register_operand (dest, <QHI:MODE>mode))
- {
- rtx di_dest = gen_rtx_REG (DImode, REGNO (dest));
- emit_insn (gen_fix_trunc<SFDF:mode>di2 (di_dest, src));
- }
- else
- {
- rtx tmp = operands[2];
- rtx tmp2 = gen_rtx_REG (<QHI:MODE>mode, REGNO (tmp));
-
- emit_insn (gen_fix_trunc<SFDF:mode>di2 (tmp, src));
- emit_move_insn (dest, tmp2);
- }
- DONE;
+ operands[3] = gen_rtx_REG (<QHI:MODE>mode, REGNO (operands[2]));
})
(define_expand "fixuns_trunc<mode>si2"
@@ -5803,48 +5808,51 @@ (define_insn "fixuns_trunc<mode>di2"
xscvdpuxds %x0,%x1"
[(set_attr "type" "fp")])
-(define_expand "fixuns_trunc<SFDF:mode><QHI:mode>2"
- [(parallel [(set (match_operand:<QHI:MODE> 0 "nonimmediate_operand")
- (unsigned_fix:QHI (match_operand:SFDF 1 "gpc_reg_operand")))
- (clobber (match_scratch:DI 2))])]
- "TARGET_P9_VECTOR && TARGET_DIRECT_MOVE_64BIT"
+;; If have ISA 3.0, QI/HImode values can go in both VSX registers and GPR
+;; registers. If we have ISA 2.07, we don't allow QI/HImode values in the
+;; vector registers, so we need to do direct moves to the GPRs, but SImode
+;; values can go in VSX registers. Keeping the direct move part through
+;; register allocation prevents the register allocator from doing a direct of
+;; the SImode value to a GPR, and then a store/load.
+(define_insn_and_split "fixuns_trunc<SFDF:mode><QHI:mode>2"
+ [(set (match_operand:<QHI:MODE> 0 "gpc_reg_operand" "=wJ,wJwK,r")
+ (unsigned_fix:QHI
+ (match_operand:SFDF 1 "gpc_reg_operand" "wJ,wJwK,wa")))
+ (clobber (match_scratch:SI 2 "=X,X,wi"))]
+ "TARGET_DIRECT_MOVE"
+ "@
+ fctiwuz %0,%1
+ xscvdpuxws %x0,%x1
+ #"
+ "&& reload_completed && int_reg_operand (operands[0], <QHI:MODE>mode)"
+ [(set (match_dup 2)
+ (unsigned_fix:SI (match_dup 1)))
+ (set (match_dup 3)
+ (match_dup 2))]
{
- if (MEM_P (operands[0]))
- operands[0] = rs6000_address_for_fpconvert (operands[0]);
-})
+ operands[3] = gen_rtx_REG (SImode, REGNO (operands[0]));
+}
+ [(set_attr "length" "4,4,8")
+ (set_attr "type" "fp")])
-(define_insn_and_split "*fixuns_trunc<SFDF:mode><QHI:mode>2_internal"
- [(set (match_operand:<QHI:MODE> 0 "reg_or_indexed_operand" "=wIwJ,rZ")
- (unsigned_fix:QHI
- (match_operand:SFDF 1 "gpc_reg_operand" "<SFDF:Fv>,<SFDF:Fv>")))
- (clobber (match_scratch:DI 2 "=X,wi"))]
- "TARGET_P9_VECTOR && TARGET_DIRECT_MOVE_64BIT"
+;; Keep the convert and store together through register allocation to prevent
+;; the register allocator from getting clever and doing a direct move to a GPR
+;; and then store for reg+offset stores.
+(define_insn_and_split "*fixuns_trunc<SFDF:mode><QHI:mode>2_mem"
+ [(set (match_operand:QHI 0 "memory_operand" "=Z")
+ (unsigned_fix:QHI (match_operand:SFDF 1 "gpc_reg_operand" "wa")))
+ (clobber (match_scratch:SI 2 "=wa"))]
+ "TARGET_P9_VECTOR"
"#"
"&& reload_completed"
- [(const_int 0)]
+ [(set (match_dup 2)
+ (unsigned_fix:SI (match_dup 1)))
+ (set (match_dup 0)
+ (match_dup 3))]
{
- rtx dest = operands[0];
- rtx src = operands[1];
-
- if (vsx_register_operand (dest, <QHI:MODE>mode))
- {
- rtx di_dest = gen_rtx_REG (DImode, REGNO (dest));
- emit_insn (gen_fixuns_trunc<SFDF:mode>di2 (di_dest, src));
- }
- else
- {
- rtx tmp = operands[2];
- rtx tmp2 = gen_rtx_REG (<QHI:MODE>mode, REGNO (tmp));
-
- emit_insn (gen_fixuns_trunc<SFDF:mode>di2 (tmp, src));
- emit_move_insn (dest, tmp2);
- }
- DONE;
+ operands[3] = gen_rtx_REG (<QHI:MODE>mode, REGNO (operands[2]));
})
-;; If -mvsx-small-integer, we can represent the FIX operation directly. On
-;; older machines, we have to use an UNSPEC to produce a SImode and move it
-;; to another location, since SImode is not allowed in vector registers.
(define_insn "*fctiw<u>z_<mode>_smallint"
[(set (match_operand:SI 0 "vsx_register_operand" "=d,wi")
(any_fix:SI (match_operand:SFDF 1 "gpc_reg_operand" "<Ff>,<Fv>")))]
@@ -14386,6 +14394,15 @@ (define_insn "fix_<mode>si2_hw"
[(set_attr "type" "vecfloat")
(set_attr "size" "128")])
+(define_insn "fix_trunc<IEEE128:mode><QHI:mode>2"
+ [(set (match_operand:<QHI:MODE> 0 "altivec_register_operand" "=v")
+ (fix:QHI
+ (match_operand:IEEE128 1 "altivec_register_operand" "v")))]
+ "TARGET_FLOAT128_HW && FLOAT128_IEEE_P (<IEEE128:MODE>mode)"
+ "xscvqpswz %0,%1"
+ [(set_attr "type" "vecfloat")
+ (set_attr "size" "128")])
+
(define_insn "fixuns_<mode>si2_hw"
[(set (match_operand:SI 0 "altivec_register_operand" "=v")
(unsigned_fix:SI (match_operand:IEEE128 1 "altivec_register_operand"
"v")))]
@@ -14394,17 +14411,40 @@ (define_insn "fixuns_<mode>si2_hw"
[(set_attr "type" "vecfloat")
(set_attr "size" "128")])
-;; Combiner pattern to prevent moving the result of converting an IEEE 128-bit
-;; floating point value to 32-bit integer to GPR in order to save it.
-(define_insn_and_split "*fix<uns>_<mode>_mem"
- [(set (match_operand:SI 0 "memory_operand" "=Z")
- (any_fix:SI (match_operand:IEEE128 1 "altivec_register_operand" "v")))
- (clobber (match_scratch:SI 2 "=v"))]
+(define_insn "fixuns_trunc<IEEE128:mode><QHI:mode>2"
+ [(set (match_operand:<QHI:MODE> 0 "altivec_register_operand" "=v")
+ (unsigned_fix:QHI
+ (match_operand:IEEE128 1 "altivec_register_operand" "v")))]
+ "TARGET_FLOAT128_HW && FLOAT128_IEEE_P (<IEEE128:MODE>mode)"
+ "xscvqpuwz %0,%1"
+ [(set_attr "type" "vecfloat")
+ (set_attr "size" "128")])
+
+;; Combiner patterns to prevent moving the result of converting an IEEE 128-bit
+;; floating point value to 8/16/32-bit integer to GPR in order to save it.
+(define_insn_and_split "*fix_trunc<IEEE128:mode><QHSI:mode>2_mem"
+ [(set (match_operand:QHSI 0 "memory_operand" "=Z")
+ (fix:QHSI
+ (match_operand:IEEE128 1 "altivec_register_operand" "v")))
+ (clobber (match_scratch:QHSI 2 "=v"))]
"TARGET_FLOAT128_HW && FLOAT128_IEEE_P (<MODE>mode)"
"#"
"&& reload_completed"
[(set (match_dup 2)
- (any_fix:SI (match_dup 1)))
+ (fix:QHSI (match_dup 1)))
+ (set (match_dup 0)
+ (match_dup 2))])
+
+(define_insn_and_split "*fixuns_trunc<IEEE128:mode><QHSI:mode>2_mem"
+ [(set (match_operand:QHSI 0 "memory_operand" "=Z")
+ (unsigned_fix:QHSI
+ (match_operand:IEEE128 1 "altivec_register_operand" "v")))
+ (clobber (match_scratch:QHSI 2 "=v"))]
+ "TARGET_FLOAT128_HW && FLOAT128_IEEE_P (<MODE>mode)"
+ "#"
+ "&& reload_completed"
+ [(set (match_dup 2)
+ (unsigned_fix:QHSI (match_dup 1)))
(set (match_dup 0)
(match_dup 2))])
Index: gcc/testsuite/gcc.target/powerpc/pr84154-1.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr84154-1.c (revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr84154-1.c (revision 0)
@@ -0,0 +1,55 @@
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } } */
+/* { dg-require-effective-target powerpc_p8vector_ok } */
+/* { dg-options "-mpower8-vector -O2" } */
+
+/* PR target/84154. Make sure conversion to char/short does not generate a
+ store and a load on ISA 2.07 and newer systems. */
+
+unsigned char
+double_to_uc (double x)
+{
+ return x;
+}
+
+signed char
+double_to_sc (double x)
+{
+ return x;
+}
+
+unsigned short
+double_to_us (double x)
+{
+ return x;
+}
+
+short
+double_to_ss (double x)
+{
+ return x;
+}
+
+unsigned int
+double_to_ui (double x)
+{
+ return x;
+}
+
+int
+double_to_si (double x)
+{
+ return x;
+}
+
+/* { dg-final { scan-assembler-times {\mextsb\M} 1 } } */
+/* { dg-final { scan-assembler-times {\mextsh\M} 1 } } */
+/* { dg-final { scan-assembler-times {\mfctiwuz\M|\mxscvdpuxws\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mfctiwz\M|\mxscvdpsxws\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mmfvsrwz\M} 6 } } */
+/* { dg-final { scan-assembler-times {\mrlwinm\M} 2 } } */
+/* { dg-final { scan-assembler-not {\mlbz\M} } } */
+/* { dg-final { scan-assembler-not {\mlhz\M} } } */
+/* { dg-final { scan-assembler-not {\mlha\M} } } */
+/* { dg-final { scan-assembler-not {\mmfvsrd\M} } } */
+/* { dg-final { scan-assembler-not {\mstw\M} } } */
Index: gcc/testsuite/gcc.target/powerpc/pr84154-2.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr84154-2.c (revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr84154-2.c (revision 0)
@@ -0,0 +1,58 @@
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } } */
+/* { dg-require-effective-target powerpc_p8vector_ok } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } {
"-mcpu=power8" } } */
+/* { dg-options "-mcpu=power8 -O2" } */
+
+/* PR target/84154. Make sure on ISA 2.07 (power8) that we store the result of
+ a conversion to char/short using an offsettable address does not generate
+ direct moves for storing 32-bit integers, but does do a direct move for
+ 8/16-bit integers. */
+
+void
+double_to_uc (double x, unsigned char *p)
+{
+ p[3] = x;
+}
+
+void
+double_to_sc (double x, signed char *p)
+{
+ p[3] = x;
+}
+
+void
+double_to_us (double x, unsigned short *p)
+{
+ p[3] = x;
+}
+
+void
+double_to_ss (double x, short *p)
+{
+ p[3] = x;
+}
+
+void
+double_to_ui (double x, unsigned int *p)
+{
+ p[3] = x;
+}
+
+void
+double_to_si (double x, int *p)
+{
+ p[3] = x;
+}
+
+/* { dg-final { scan-assembler-times {\mfctiwuz\M|\mxscvdpuxws\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mfctiwz\M|\mxscvdpsxws\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mmfvsrwz\M} 4 } } */
+/* { dg-final { scan-assembler-times {\mstfiwx\M|\mstxsiwx\M} 2 } } */
+/* { dg-final { scan-assembler-times {\mstb\M} 2 } } */
+/* { dg-final { scan-assembler-times {\msth\M} 2 } } */
+/* { dg-final { scan-assembler-not {\mlbz\M} } } */
+/* { dg-final { scan-assembler-not {\mlhz\M} } } */
+/* { dg-final { scan-assembler-not {\mlha\M} } } */
+/* { dg-final { scan-assembler-not {\mmfvsrd\M} } } */
+/* { dg-final { scan-assembler-not {\mstw\M} } } */
Index: gcc/testsuite/gcc.target/powerpc/pr84154-3.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr84154-3.c (revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr84154-3.c (revision 0)
@@ -0,0 +1,60 @@
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } } */
+/* { dg-require-effective-target powerpc_p9vector_ok } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } {
"-mcpu=power9" } } */
+/* { dg-options "-mcpu=power9 -O2" } */
+
+/* PR target/84154. Make sure on ISA 3.0 we store the result of a conversion
+ to char/short using an offsettable address does not generate direct moves
+ for storing 8/16/32-bit integers. */
+
+void
+double_to_uc (double x, unsigned char *p)
+{
+ p[3] = x;
+}
+
+void
+double_to_sc (double x, signed char *p)
+{
+ p[3] = x;
+}
+
+void
+double_to_us (double x, unsigned short *p)
+{
+ p[3] = x;
+}
+
+void
+double_to_ss (double x, short *p)
+{
+ p[3] = x;
+}
+
+void
+double_to_ui (double x, unsigned int *p)
+{
+ p[3] = x;
+}
+
+void
+double_to_si (double x, int *p)
+{
+ p[3] = x;
+}
+
+/* { dg-final { scan-assembler-times {\maddi\M} 6 } } */
+/* { dg-final { scan-assembler-times {\mfctiwuz\M|\mxscvdpuxws\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mfctiwz\M|\mxscvdpsxws\M} 3 } } */
+/* { dg-final { scan-assembler-times {\mstfiwx\M|\mstxsiwx\M} 2 } } */
+/* { dg-final { scan-assembler-times {\mstxsibx\M} 2 } } */
+/* { dg-final { scan-assembler-times {\mstxsihx\M} 2 } } */
+/* { dg-final { scan-assembler-not {\mlbz\M} } } */
+/* { dg-final { scan-assembler-not {\mlhz\M} } } */
+/* { dg-final { scan-assembler-not {\mlha\M} } } */
+/* { dg-final { scan-assembler-not {\mmfvsrwz\M} } } */
+/* { dg-final { scan-assembler-not {\mmfvsrd\M} } } */
+/* { dg-final { scan-assembler-not {\mstw\M} } } */
+/* { dg-final { scan-assembler-not {\mstb\M} } } */
+/* { dg-final { scan-assembler-not {\msth\M} } } */
