RFA: one more version of the patch for PR61360

[Richard Sandiford]

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.

Thanks,
Richard