Re: [PATCH] Fix -march=bdver1 ICE on int to float conversion (PR target/84844)

[Uros Bizjak <ubizjak at gmail dot com>]

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