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

[Richard Sandiford <rdsandiford at googlemail dot com>]

Uros Bizjak <ubizjak@gmail.com> writes:
> 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.

I was trying to avoid having "enable" in the names because the new
attributes would only be strong optimisation hints whereas "enabled"
is a correctness thing.  We'd need to ignore the new attributes when
constraining an existing instruction (which might have been optimised
the opposite way and then moved) but we can never ignore "enabled".

E.g. to take two examples, when constrain_operands is used like this:

	if (! constrain_operands (1))
	  fatal_insn_not_found (insn);

then only the enabled attribute matters, whereas for validate_change
I think we'd want to take the new attributes into account as well.

"good_for_" wasn't a great name though.  Better suggestions are
definitely welcome.

Thanks,
Richard