[expr.c] PR 65358 Avoid clobbering partial argument during sibcall

Hi Jeff,

On 12/05/15 23:04, Jeff Law wrote:
> On 05/11/2015 03:28 AM, Kyrill Tkachov wrote:
>>> The more I think about this, the more I think it's an ugly can of
>>> worms and maybe we should just disable sibcalls for partial
>>> arguments.  I doubt it's a big performance issue in general.
>>
>> We already have quite a bit of code in calls.c to detect cases with
>> partial argument overlap for the
>> explicit purpose of allowing sibcalls when partial arguments occur in
>> the general case. However, that
>> code only detects when a partial argument overlaps with other arguments
>> in a call. In this PR the
>> partial argument overlaps with itself. It would be a shame to disable
>> sibcalls for all partial arguments
>> when there is already infrastructure in place to handle them.
> I didn't even realize we had support for partial arguments in sibcalls.   Ah, Kazu added that in 2005, I totally missed it.  I probably would have suggested failing the sibcall for those cases back then too...
>
> Is there any way to re-use that infrastructure to deal with the case at hand?
>
>
>
>>
>>>
>>>
>>> In addition to the argument/stack direction stuff, I've been pondering
>>> the stack/frame/arg pointer issues.  Your approach assumes that the
>>> incoming and outgoing areas are always referenced off the same base
>>> register.  If they aren't, then the routine returns no overlap.
>>>
>>> But we'd need to consider the case where we have a reference to the
>>> arg or frame pointer which later gets rewritten into a stack pointer
>>> relative address.
>>>
>>> Is it too late at the point were you do the checks to reject the
>>> sibling call?  If not, then maybe the overlap routine should return a
>>> tri-state.  No overlap, overlap, don't know.  The last would be used
>>> when the two addresses use a different register.
>>
>> Ok, here is my attempt at that. The overlap functions returns -2 when it
>> cannot staticall compare the
>> two pointers (i.e. when the base registers are different) and the caller
>> then disables sibcalls.
>> The code in calls.c that calls this code will undo any emitted
>> instructions in the meantime if sibcall
>> optimisation fails. This required me to change the type of
>> emit_push_insn to bool and add an extra
>> parameter, so this patch touches a bit more code than the original version.
>>
>> Bootstrapped on x86_64 and tested on arm. The testcase in this PR still
>> performs a sibcall correctly on arm.
>>
>> What do you think of this?
>>
>> Thanks,
>> Kyrill
>>
>>
>> 2015-05-11  Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>>
>>      PR target/65358
>>      * expr.c (memory_load_overlap): New function.
>>      (emit_push_insn): When pushing partial args to the stack would
>>      clobber the register part load the overlapping part into a pseudo
>>      and put it into the hard reg after pushing.  Change return type
>>      to bool.  Add bool argument.
>>      * expr.h (emit_push_insn): Change return type to bool.
>>      Add bool argument.
>>      * calls.c (expand_call): Cancel sibcall optimisation when encountering
>>      partial argument on targets with ARGS_GROW_DOWNWARD and
>>      !STACK_GROWS_DOWNWARD.
>>      (emit_library_call_value_1): Update callsite of emit_push_insn.
>>      (store_one_arg): Likewise.
>>
>>
>> 2015-05-11  Honggyu Kim <hong.gyu.kim@lge.com>
>>
>>      PR target/65358
>>      * gcc.dg/pr65358.c: New test.
>>
>>>
>>> Jeff
>>>
>>>
>>
>>
>> expr.patch
>>
>>
>> commit 5b596f10846b6d3b143442a306801c8262d8b10a
>> Author: Kyrylo Tkachov<kyrylo.tkachov@arm.com>
>> Date:   Wed Mar 18 13:42:37 2015 +0000
>>
>>      [expr.c] PR 65358 Avoid clobbering partial argument during sibcall
>>
>> diff --git a/gcc/calls.c b/gcc/calls.c
>> index caa7d60..81ef2c9 100644
>> --- a/gcc/calls.c
>> +++ b/gcc/calls.c
>> @@ -3225,6 +3225,13 @@ expand_call (tree exp, rtx target, int ignore)
>>           {
>>             rtx_insn *before_arg = get_last_insn ();
>>
>> +         /* On targets with weird calling conventions (e.g. PA) it's
>> +        hard to ensure that all cases of argument overlap between
>> +        stack and registers work.  Play it safe and bail out. */
>> +#if defined (ARGS_GROW_DOWNWARD) && !defined (STACK_GROWS_DOWNWARD)
>> +          sibcall_failure = 1;
>> +          break;
>> +#endif
> So we're trying to get away from this kind of conditional compilation.
>
> Instead we want to write
>
> if (ARGS_GROW_DOWNWARD && !STACK_GROWS_DOWNWARD)
>
> ARGS_GROW_DOWNWARD is already a testable value.  But STACK_GROWS_DOWNWARD is not.  The way folks have been dealing with this is something like this after the #includes:
>
> /* Redefine STACK_GROWS_DOWNWARD in terms of 0 or 1.  */
> #ifdef STACK_GROWS_DOWNWARD
> # undef STACK_GROWS_DOWNWARD
> # define STACK_GROWS_DOWNWARD 1
> #else
> # define STACK_GROWS_DOWNWARD 0
> #endif
>
>
> With that in place you can change the test into the more desirable
> if (ARGS_GROW_DOWNWARD && !STACK_GROWS_DOWNWARD)
>
>
>
>> diff --git a/gcc/expr.c b/gcc/expr.c
>> index 25aa11f..712fa0b 100644
>> --- a/gcc/expr.c
>> +++ b/gcc/expr.c
>> @@ -4121,12 +4121,35 @@ emit_single_push_insn (machine_mode mode, rtx x, tree type)
>>   }
>>   #endif
>>
>> +/* If reading SIZE bytes from X will end up reading from
>> +   Y return the number of bytes that overlap.  Return -1
>> +   if there is no overlap or -2 if we can't determing
> s/determing/determine/
>
>> +   partial argument during a sibcall optimisation (as specified by
> s/optimisation/optimization/
>
>
> I guess I'm still not real comfortable that we've got the issues here resolved and if we're going to try and support this case, then re-using the existing infrastructure would be better.
>
> I'll approve with the changes noted above, but would ask that you look into trying to re-use the existing infrastructure as a follow-up.

Thanks, I'm attaching what I committed with r223753.
I've raised PR 66307 for the infrastructure work.

Thanks again for helping beat this into shape.

Kyrill

>
> Jeff
>
>

[expr.c] PR 65358 Avoid clobbering partial argument during sibcall

Commit Message

Patch