On Wed, Mar 14, 2018 at 9:36 AM, Uros Bizjak <ubiz...@gmail.com> wrote:
> On Tue, Mar 13, 2018 at 9:30 PM, Jakub Jelinek <ja...@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 <ja...@redhat.com>
>>
>> PR target/84844
>> Revert
>> 2017-04-20 Uros Bizjak <ubiz...@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
