On 09/23/2014 02:07 AM, Uros Bizjak wrote:
> On Tue, Sep 23, 2014 at 3:26 AM, Vladimir Makarov <vmaka...@redhat.com> wrote:
>> The previous patch to solve PR61360 fixed the problem in IRA (it was
>> easier for me to do as I know the code well)
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61360
>>
>> Although imo it was an ok fix, Richard expressed concerns with the patch
>> and the practice to have different enable attribute values depending on the
>> current pass.
>>
>> I don't understand why "x,m" alternative is better to "x,r" and "x,r"
>> should be disabled. Even if the path from general regs to sse regs is slow
>> (usually such slow path is implemented internally by micro-architecture
>> through cache). "x,r" alternative results in only smaller insns (including
>> number of insns) with probably the same time for the movement. So "x,r"
>> should be at least no slower, insn cache should have more locality, and less
>> overhead for decoding/translating insns.
>>
>> Here I propose another solution avoiding to have different enable
>> attribute values.
>>
>> The patch was successfully bootstrapped on x86/x86-64 and tested with and
>> without -march=amdfam10 (actually the patch results in 2 less failures when
>> -march=amdfam10 were used).
>>
>> Uros, is i386.md change ok for the trunk?
> I don't think so. This would be a regression, since 4.8 (and later
> versions until Richard's patch) were able to handle this functionality
> just fine. Please also note that there are a couple of other patterns
> with the same problem, that is using ("nonimmediate_operand" "m")
> constraint.
>
> Please see PR 60704, comment 7 [1]. If LRA is able to fixup
> ("nonimmediate_operand" "m") to a memory from a register, then other
> RTL infrastructure should also be updated for this functionality. IMO,
> recog should be fixed/enhanced, so pseudo registers will also satisfy
> ("nonimmediate_operand" "m") constraint before LRA.
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60704#c7
>
>
Uros, my patch does not result in PR60704 (I tested it before submitting
the patch).
Also I don't understand why you are mentioning only "m" alternative as I
enabled another alternative "x,r" in original pattern (alternative "1"
in other words alternative in the middle).
(define_insn "*float<SWI48:mode><MODEF:mode>2_sse"
[(set (match_operand:MODEF 0 "register_operand" "=f,x,x")
(float:MODEF
(match_operand:SWI48 1 "nonimmediate_operand" "m,r,m")))]
"SSE_FLOAT_MODE_P (<MODEF:MODE>mode) && TARGET_SSE_MATH"
"@
fild%Z1\t%1
%vcvtsi2<MODEF:ssemodesuffix><SWI48:rex64suffix>\t{%1, %d0|%d0, %1}
%vcvtsi2<MODEF:ssemodesuffix><SWI48:rex64suffix>\t{%1, %d0|%d0, %1}"
[(set_attr "type" "fmov,sseicvt,sseicvt")
(set_attr "prefix" "orig,maybe_vex,maybe_vex")
(set_attr "mode" "<MODEF:MODE>")
(set (attr "prefix_rex")
(if_then_else
(and (eq_attr "prefix" "maybe_vex")
(match_test "<SWI48:MODE>mode == DImode"))
(const_string "1")
(const_string "*")))
(set_attr "unit" "i387,*,*")
(set_attr "athlon_decode" "*,double,direct")
(set_attr "amdfam10_decode" "*,vector,double")
(set_attr "bdver1_decode" "*,double,direct")
(set_attr "fp_int_src" "true")
(set (attr "enabled")
(cond [(eq_attr "alternative" "0")
(symbol_ref "TARGET_MIX_SSE_I387
&& X87_ENABLE_FLOAT (<MODEF:MODE>mode,
<SWI48:MODE>mode)")
(eq_attr "alternative" "1")
/* ??? For sched1 we need constrain_operands to be able to
select an alternative. Leave this enabled before RA. */
(symbol_ref "TARGET_INTER_UNIT_CONVERSIONS
|| optimize_function_for_size_p (cfun)
|| !(reload_completed
|| reload_in_progress
|| lra_in_progress)")
]
(symbol_ref "true")))
])
What you are saying would be true if I enabled "x,m" and it was single
alternative in insn because constrain_operands rejects such instruction until
pseudo is changed by memory.
You are right constrain_operands is not upto LRA possibilities and we should
make the following change:
Index: recog.c
===================================================================
--- recog.c (revision 215337)
+++ recog.c (working copy)
@@ -2639,7 +2639,10 @@ constrain_operands (int strict)
|| (strict < 0 && CONSTANT_P (op))
/* During reload, accept a pseudo */
|| (reload_in_progress && REG_P (op)
- && REGNO (op) >= FIRST_PSEUDO_REGISTER)))
+ && REGNO (op) >= FIRST_PSEUDO_REGISTER)
+ /* LRA can put reg value into memory if
+ it is necessary. */
+ || (strict <= 0 && targetm.lra_p () && REG_P
(op)))
win = 1;
else if (insn_extra_address_constraint (cn)
/* Every address operand can be reloaded to fit. */
But that is a different story (for insns with single alternative containing
only "m").
I guess I should submit such change for recog.c as a separate patch.
