Re: RFA: one more version of the patch for PR61360

[Uros Bizjak <ubizjak at gmail dot com>]

On Sat, Oct 4, 2014 at 12:49 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Uros Bizjak <ubizjak@gmail.com> writes:
>> On Thu, Oct 2, 2014 at 10:13 PM, Vladimir Makarov <vmakarov@redhat.com> wrote:
>>
>>>>>> I guess we achieved the consensus about the following patch to fix
>>>>>> PR61360
>>>>>>
>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61360
>>>>>>
>>>>>> The patch was successfully bootstrapped and tested (w/wo
>>>>>> -march=amdfam10) on x86/x86-64.
>>>>>>
>>>>>> Is it ok to commit to trunk?
>>>>>
>>>>>
>>>>> I've tested your patch and unfortunately it doesn't work:
>>>>>
>>>>> In file included from
>>>>> /var/tmp/moz-build-dir/js/src/shell/Unified_cpp_js_src_shell0.cpp:15:0:
>>>>> /var/tmp/mozilla-central/js/src/shell/js.cpp: In function âvoid
>>>>> Process(JSContext*, JSObject*, const char*, bool)â:
>>>>> /var/tmp/mozilla-central/js/src/shell/js.cpp:592:1: internal compiler
>>>>> error: in lra_update_insn_recog_data, at lra.c:1221
>>>>>   }
>>>>>   ^
>>>>> 0xa9d9ec lra_update_insn_recog_data(rtx_insn*)
>>>>>          ../../gcc/gcc/lra.c:1220
>>>>> 0xab450f eliminate_regs_in_insn
>>>>>          ../../gcc/gcc/lra-eliminations.c:1077
>>>>> 0xab450f process_insn_for_elimination
>>>>>          ../../gcc/gcc/lra-eliminations.c:1344
>>>>> 0xab450f lra_eliminate(bool, bool)
>>>>>          ../../gcc/gcc/lra-eliminations.c:1408
>>>>> 0xa9f2da lra(_IO_FILE*)
>>>>>          ../../gcc/gcc/lra.c:2270
>>>>> 0xa5d659 do_reload
>>>>>          ../../gcc/gcc/ira.c:5311
>>>>> 0xa5d659 execute
>>>>>          ../../gcc/gcc/ira.c:5470
>>>>
>>>>
>>>> Testcase is attached:
>>>>
>>>>   % g++ -c -march=amdfam10 -w -O2 js.ii
>>>> js.ii: In function âvoid RunFile(C)â:
>>>> js.ii:64:1: internal compiler error: in lra_update_insn_recog_data, at
>>>> lra.c:1221
>>>>
>>>
>>> Thanks for reporting this, Marcus.  The problem now is in
>>> optimize_function_for_size_p.  It is false, when we define and cache enable
>>> attributes at early stages (instantation of virtual regs) and true later.
>>>
>>> It is returning us to the same problem.  I believe that we should not have
>>> enable attributes depending on optimization options or on the current
>>> running pass if we don't want the current solution in the trunk (with
>>> recog_init).  Setting right value for optimize_function_for_size_p does not
>>> solve the problem as we can have different options for different functions
>>> in the same compilation file.
>>>
>>> So minimal solution would be removing optimize_function_for_size_p from the
>>> attribute definition.  But I guess we could remove all condition.
>>> Unfortunately, Ganesh did not post is it really beneficial to switch off
>>> this alternative for AMD CPUs even if AMD optimization guide recommends it.
>>
>> I propose to split the pattern into size-optimized and normal pattern.
>> The patch implements this proposal.
>
> An alternative would be to add two new enabled-like attributes,
> "good_for_size" and "good_for_speed", that say whether it is efficient
> to use a particular alternative.  These attributes wouldn't ever stop
> an existing instruction from being recognised; they would just say
> whether the RA and optimisers should consider the alternative when
> optimising for size or speed respectively.  The attributes would have
> the same restrictions as the enabled attribute and could be cached in
> the same way.
>
> The "enabled" attribute would then be purely about whether an
> instruction is available, not whether it's efficient in a particular
> situation.
>
> The main advantage is that it would be possible to make the size/speed
> choice at a basic block level rather than a function level.  In the worst
> case we might move an instruction from a hot block to a cold block or
> vice versa, but with intelligent spilling that shouldn't happen too
> often and at least it wouldn't trigger an ICE.
>
> If that sounds OK, I'll try to get something together next week.
> I think Ramana said he had a use for this on ARM too.

I think that this would be way better than duplication of patterns.
Perhaps we can name these attributes "enable_for_size" and
"enable_for_speed", and have them in addition to "enable" attribute.
The final enable condition would be an union of "enable",
"enable_for_speed" and "enable_for_size" attributes.

Uros.