diff mbox series

Fix -march=bdver1 ICE on int to float conversion (PR target/84844)

Message ID 20180313203036.GE8577@tucnak
State New
Headers show
Series Fix -march=bdver1 ICE on int to float conversion (PR target/84844) | expand

Commit Message

Jakub Jelinek March 13, 2018, 8:30 p.m. UTC 
Hi!

As mentioned in bugzilla, when e.g. sel-sched queries (indirectly) before reload
some attributes like get_attr_type that depend on alternatives, GCC attempts
to constrain the operands in non-strict mode, which implies that if
reg_class_for_constraint doesn't return NO_REGS, it is ok, otherwise the
constraint needs to match (the actual code is more complex of course).
The *float<SWI48:mode><MODEF:mode>2_mixed pattern has different type
attributes between different alternatives, uses nonimmediate_operand for the
input and uses "m" constraint for it in all but one alternative; in that
alternative it has "r" constraint for the input and "Yc" for output, which
depending on tuning is either same as "v" or NO_REGS.  So, on those tunings
even in non-strict mode, if the input is a REG we fail to constrain the insn
and ICE.

The following patch fixes it by reverting the offending patch (as asked in
the PR), even with the patch reverted the reported issue doesn't reproduce
and in theory there is nothing wrong on emitting direct conversions even in
these tunings in cold blocks, the hw supports it, just it is slow, but also
smaller.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

As mentioned in the PR, another alternative that works is adding another
alternative next to that Yc <- r, e.g. !???*v <- r, which will allow the
pre-reload attribute queries, but will very likely not be used otherwise.

2018-03-13  Jakub Jelinek  <jakub@redhat.com>

	PR target/84844
	Revert
	2017-04-20  Uros Bizjak  <ubizjak@gmail.com>

	PR target/78090
	* config/i386/constraints.md (Yc): New register constraint.
	* config/i386/i386.md (*float<SWI48:mode><MODEF:mode>2_mixed):
	Use Yc constraint for alternative 2 of operand 0.  Remove
	preferred_for_speed attribute.

	* gcc.target/i386/pr84844.c: New test.


	Jakub

Comments

Uros Bizjak March 14, 2018, 8:36 a.m. UTC | #1 
On Tue, Mar 13, 2018 at 9:30 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> As mentioned in bugzilla, when e.g. sel-sched queries (indirectly) before reload
> some attributes like get_attr_type that depend on alternatives, GCC attempts
> to constrain the operands in non-strict mode, which implies that if
> reg_class_for_constraint doesn't return NO_REGS, it is ok, otherwise the
> constraint needs to match (the actual code is more complex of course).
> The *float<SWI48:mode><MODEF:mode>2_mixed pattern has different type
> attributes between different alternatives, uses nonimmediate_operand for the
> input and uses "m" constraint for it in all but one alternative; in that
> alternative it has "r" constraint for the input and "Yc" for output, which
> depending on tuning is either same as "v" or NO_REGS.  So, on those tunings
> even in non-strict mode, if the input is a REG we fail to constrain the insn
> and ICE.
>
> The following patch fixes it by reverting the offending patch (as asked in
> the PR), even with the patch reverted the reported issue doesn't reproduce
> and in theory there is nothing wrong on emitting direct conversions even in
> these tunings in cold blocks, the hw supports it, just it is slow, but also
> smaller.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> As mentioned in the PR, another alternative that works is adding another
> alternative next to that Yc <- r, e.g. !???*v <- r, which will allow the
> pre-reload attribute queries, but will very likely not be used otherwise.
>
> 2018-03-13  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/84844
>         Revert
>         2017-04-20  Uros Bizjak  <ubizjak@gmail.com>
>
>         PR target/78090
>         * config/i386/constraints.md (Yc): New register constraint.
>         * config/i386/i386.md (*float<SWI48:mode><MODEF:mode>2_mixed):
>         Use Yc constraint for alternative 2 of operand 0.  Remove
>         preferred_for_speed attribute.
>
>         * gcc.target/i386/pr84844.c: New test.

OK.

Perhaps some time in future, we should change all these inter-unit
constraints to use preferred_for_speed attribute. As with the attached
patch, these insn are not invalid instructions, so we can emit them in
certain cases (-Os), even for AMD targets. Conditional register
constraints made sense were

--- gcc/config/i386/constraints.md.jj   2018-02-26 20:49:57.299331387 +0100
> +++ gcc/config/i386/constraints.md      2018-03-13 13:47:22.285093035 +0100
> @@ -99,7 +99,6 @@ (define_register_constraint "w" "TARGET_
>
>  ;; We use the Y prefix to denote any number of conditional register sets:
>  ;;  z  First SSE register.
> -;;  c  SSE inter-unit conversions enabled
>  ;;  i  SSE2 inter-unit moves to SSE register enabled
>  ;;  j  SSE2 inter-unit moves from SSE register enabled
>  ;;  d  any EVEX encodable SSE register for AVX512BW target or any SSE register
> @@ -124,10 +123,6 @@ (define_register_constraint "w" "TARGET_
>  (define_register_constraint "Yz" "TARGET_SSE ? SSE_FIRST_REG : NO_REGS"
>   "First SSE register (@code{%xmm0}).")
>
> -(define_register_constraint "Yc"
> - "TARGET_SSE && TARGET_INTER_UNIT_CONVERSIONS ? ALL_SSE_REGS : NO_REGS"
> - "@internal Any SSE register, when SSE and inter-unit conversions are enabled.")
> -
>  (define_register_constraint "Yi"
>   "TARGET_SSE2 && TARGET_INTER_UNIT_MOVES_TO_VEC ? ALL_SSE_REGS : NO_REGS"
>   "@internal Any SSE register, when SSE2 and inter-unit moves to vector registers are enabled.")
> --- gcc/config/i386/i386.md.jj  2018-03-13 13:40:44.082903460 +0100
> +++ gcc/config/i386/i386.md     2018-03-13 13:47:22.284093034 +0100
> @@ -5325,7 +5325,7 @@ (define_expand "float<SWI48:mode><MODEF:
>  })
>
>  (define_insn "*float<SWI48:mode><MODEF:mode>2_mixed"
> -  [(set (match_operand:MODEF 0 "register_operand" "=f,Yc,v")
> +  [(set (match_operand:MODEF 0 "register_operand" "=f,v,v")
>         (float:MODEF
>           (match_operand:SWI48 1 "nonimmediate_operand" "m,r,m")))]
>    "SSE_FLOAT_MODE_P (<MODEF:MODE>mode) && TARGET_SSE_MATH"
> @@ -5354,6 +5354,10 @@ (define_insn "*float<SWI48:mode><MODEF:m
>                             && X87_ENABLE_FLOAT (<MODEF:MODE>mode,
>                                                  <SWI48:MODE>mode)")
>             ]
> +           (symbol_ref "true")))
> +   (set (attr "preferred_for_speed")
> +     (cond [(eq_attr "alternative" "1")
> +              (symbol_ref "TARGET_INTER_UNIT_CONVERSIONS")]
>             (symbol_ref "true")))])
>
>  (define_insn "*float<SWI48x:mode><MODEF:mode>2_i387"
> --- gcc/testsuite/gcc.target/i386/pr84844.c.jj  2018-03-13 13:12:50.569130703 +0100
> +++ gcc/testsuite/gcc.target/i386/pr84844.c     2018-03-13 12:21:04.553643164 +0100
> @@ -0,0 +1,10 @@
> +/* PR target/84844 */
> +/* { dg-do compile } */
> +/* { dg-options "-march=bdver1 -O2 -fschedule-insns -fselective-scheduling" } */
> +
> +double
> +foo (int *x, int y, int z)
> +{
> +  *x = y;
> +  return z;
> +}
>
>         Jakub
Uros Bizjak March 14, 2018, 8:37 a.m. UTC | #2 
On Wed, Mar 14, 2018 at 9:36 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Mar 13, 2018 at 9:30 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> Hi!
>>
>> As mentioned in bugzilla, when e.g. sel-sched queries (indirectly) before reload
>> some attributes like get_attr_type that depend on alternatives, GCC attempts
>> to constrain the operands in non-strict mode, which implies that if
>> reg_class_for_constraint doesn't return NO_REGS, it is ok, otherwise the
>> constraint needs to match (the actual code is more complex of course).
>> The *float<SWI48:mode><MODEF:mode>2_mixed pattern has different type
>> attributes between different alternatives, uses nonimmediate_operand for the
>> input and uses "m" constraint for it in all but one alternative; in that
>> alternative it has "r" constraint for the input and "Yc" for output, which
>> depending on tuning is either same as "v" or NO_REGS.  So, on those tunings
>> even in non-strict mode, if the input is a REG we fail to constrain the insn
>> and ICE.
>>
>> The following patch fixes it by reverting the offending patch (as asked in
>> the PR), even with the patch reverted the reported issue doesn't reproduce
>> and in theory there is nothing wrong on emitting direct conversions even in
>> these tunings in cold blocks, the hw supports it, just it is slow, but also
>> smaller.
>>
>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>
>> As mentioned in the PR, another alternative that works is adding another
>> alternative next to that Yc <- r, e.g. !???*v <- r, which will allow the
>> pre-reload attribute queries, but will very likely not be used otherwise.
>>
>> 2018-03-13  Jakub Jelinek  <jakub@redhat.com>
>>
>>         PR target/84844
>>         Revert
>>         2017-04-20  Uros Bizjak  <ubizjak@gmail.com>
>>
>>         PR target/78090
>>         * config/i386/constraints.md (Yc): New register constraint.
>>         * config/i386/i386.md (*float<SWI48:mode><MODEF:mode>2_mixed):
>>         Use Yc constraint for alternative 2 of operand 0.  Remove
>>         preferred_for_speed attribute.
>>
>>         * gcc.target/i386/pr84844.c: New test.
>
> OK.
>
> Perhaps some time in future, we should change all these inter-unit
> constraints to use preferred_for_speed attribute. As with the attached
> patch, these insn are not invalid instructions, so we can emit them in
> certain cases (-Os), even for AMD targets. Conditional register
> constraints made sense ...

... before preferred_for_... infrastructure was developed.

Uros.

>
> --- gcc/config/i386/constraints.md.jj   2018-02-26 20:49:57.299331387 +0100
>> +++ gcc/config/i386/constraints.md      2018-03-13 13:47:22.285093035 +0100
>> @@ -99,7 +99,6 @@ (define_register_constraint "w" "TARGET_
>>
>>  ;; We use the Y prefix to denote any number of conditional register sets:
>>  ;;  z  First SSE register.
>> -;;  c  SSE inter-unit conversions enabled
>>  ;;  i  SSE2 inter-unit moves to SSE register enabled
>>  ;;  j  SSE2 inter-unit moves from SSE register enabled
>>  ;;  d  any EVEX encodable SSE register for AVX512BW target or any SSE register
>> @@ -124,10 +123,6 @@ (define_register_constraint "w" "TARGET_
>>  (define_register_constraint "Yz" "TARGET_SSE ? SSE_FIRST_REG : NO_REGS"
>>   "First SSE register (@code{%xmm0}).")
>>
>> -(define_register_constraint "Yc"
>> - "TARGET_SSE && TARGET_INTER_UNIT_CONVERSIONS ? ALL_SSE_REGS : NO_REGS"
>> - "@internal Any SSE register, when SSE and inter-unit conversions are enabled.")
>> -
>>  (define_register_constraint "Yi"
>>   "TARGET_SSE2 && TARGET_INTER_UNIT_MOVES_TO_VEC ? ALL_SSE_REGS : NO_REGS"
>>   "@internal Any SSE register, when SSE2 and inter-unit moves to vector registers are enabled.")
>> --- gcc/config/i386/i386.md.jj  2018-03-13 13:40:44.082903460 +0100
>> +++ gcc/config/i386/i386.md     2018-03-13 13:47:22.284093034 +0100
>> @@ -5325,7 +5325,7 @@ (define_expand "float<SWI48:mode><MODEF:
>>  })
>>
>>  (define_insn "*float<SWI48:mode><MODEF:mode>2_mixed"
>> -  [(set (match_operand:MODEF 0 "register_operand" "=f,Yc,v")
>> +  [(set (match_operand:MODEF 0 "register_operand" "=f,v,v")
>>         (float:MODEF
>>           (match_operand:SWI48 1 "nonimmediate_operand" "m,r,m")))]
>>    "SSE_FLOAT_MODE_P (<MODEF:MODE>mode) && TARGET_SSE_MATH"
>> @@ -5354,6 +5354,10 @@ (define_insn "*float<SWI48:mode><MODEF:m
>>                             && X87_ENABLE_FLOAT (<MODEF:MODE>mode,
>>                                                  <SWI48:MODE>mode)")
>>             ]
>> +           (symbol_ref "true")))
>> +   (set (attr "preferred_for_speed")
>> +     (cond [(eq_attr "alternative" "1")
>> +              (symbol_ref "TARGET_INTER_UNIT_CONVERSIONS")]
>>             (symbol_ref "true")))])
>>
>>  (define_insn "*float<SWI48x:mode><MODEF:mode>2_i387"
>> --- gcc/testsuite/gcc.target/i386/pr84844.c.jj  2018-03-13 13:12:50.569130703 +0100
>> +++ gcc/testsuite/gcc.target/i386/pr84844.c     2018-03-13 12:21:04.553643164 +0100
>> @@ -0,0 +1,10 @@
>> +/* PR target/84844 */
>> +/* { dg-do compile } */
>> +/* { dg-options "-march=bdver1 -O2 -fschedule-insns -fselective-scheduling" } */
>> +
>> +double
>> +foo (int *x, int y, int z)
>> +{
>> +  *x = y;
>> +  return z;
>> +}
>>
>>         Jakub
diff mbox series

Patch

--- gcc/config/i386/constraints.md.jj	2018-02-26 20:49:57.299331387 +0100
+++ gcc/config/i386/constraints.md	2018-03-13 13:47:22.285093035 +0100
@@ -99,7 +99,6 @@  (define_register_constraint "w" "TARGET_
 
 ;; We use the Y prefix to denote any number of conditional register sets:
 ;;  z	First SSE register.
-;;  c	SSE inter-unit conversions enabled
 ;;  i	SSE2 inter-unit moves to SSE register enabled
 ;;  j	SSE2 inter-unit moves from SSE register enabled
 ;;  d	any EVEX encodable SSE register for AVX512BW target or any SSE register
@@ -124,10 +123,6 @@  (define_register_constraint "w" "TARGET_
 (define_register_constraint "Yz" "TARGET_SSE ? SSE_FIRST_REG : NO_REGS"
  "First SSE register (@code{%xmm0}).")
 
-(define_register_constraint "Yc"
- "TARGET_SSE && TARGET_INTER_UNIT_CONVERSIONS ? ALL_SSE_REGS : NO_REGS"
- "@internal Any SSE register, when SSE and inter-unit conversions are enabled.")
-
 (define_register_constraint "Yi"
  "TARGET_SSE2 && TARGET_INTER_UNIT_MOVES_TO_VEC ? ALL_SSE_REGS : NO_REGS"
  "@internal Any SSE register, when SSE2 and inter-unit moves to vector registers are enabled.")
--- gcc/config/i386/i386.md.jj	2018-03-13 13:40:44.082903460 +0100
+++ gcc/config/i386/i386.md	2018-03-13 13:47:22.284093034 +0100
@@ -5325,7 +5325,7 @@  (define_expand "float<SWI48:mode><MODEF:
 })
 
 (define_insn "*float<SWI48:mode><MODEF:mode>2_mixed"
-  [(set (match_operand:MODEF 0 "register_operand" "=f,Yc,v")
+  [(set (match_operand:MODEF 0 "register_operand" "=f,v,v")
 	(float:MODEF
 	  (match_operand:SWI48 1 "nonimmediate_operand" "m,r,m")))]
   "SSE_FLOAT_MODE_P (<MODEF:MODE>mode) && TARGET_SSE_MATH"
@@ -5354,6 +5354,10 @@  (define_insn "*float<SWI48:mode><MODEF:m
                            && X87_ENABLE_FLOAT (<MODEF:MODE>mode,
                                                 <SWI48:MODE>mode)")
            ]
+           (symbol_ref "true")))
+   (set (attr "preferred_for_speed")
+     (cond [(eq_attr "alternative" "1")
+              (symbol_ref "TARGET_INTER_UNIT_CONVERSIONS")]
            (symbol_ref "true")))])
 
 (define_insn "*float<SWI48x:mode><MODEF:mode>2_i387"
--- gcc/testsuite/gcc.target/i386/pr84844.c.jj	2018-03-13 13:12:50.569130703 +0100
+++ gcc/testsuite/gcc.target/i386/pr84844.c	2018-03-13 12:21:04.553643164 +0100
@@ -0,0 +1,10 @@ 
+/* PR target/84844 */
+/* { dg-do compile } */
+/* { dg-options "-march=bdver1 -O2 -fschedule-insns -fselective-scheduling" } */
+
+double
+foo (int *x, int y, int z)
+{
+  *x = y;
+  return z;
+}