Re: [PATCH i386] Extend sibcall peepholes to allow source in %eax

[Jeff Law <law@redhat.com>]

On 05/11/2015 01:46 PM, Uros Bizjak wrote:
> On Mon, May 11, 2015 at 8:00 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> On Sun, 10 May 2015, Jan Hubicka wrote:
>>>
>>>>> On i386, peepholes that transform memory load and register-indirect jump into
>>>>> memory-indirect jump are overly restrictive in that they don't allow combining
>>>>> when the jump target is loaded into %eax, and the called function returns a
>>>>> value (also in %eax, so it's not dead after the call).  Fix this by checking
>>>>> for same source and output register operands separately.
>>>>>
>>>>> OK?
>>>>>    * config/i386/i386.md (sibcall_value_memory): Extend peepholes to
>>>>>    allow memory address in %eax.
>>>>>    (sibcall_value_pop_memory): Likewise.
>>>>
>>>> Why do we need the check for liveness after all?  There is SIBLING_CALL_P
>>>> (peep2_next_insn (1)) so we know that the function terminates by the call
>>>> and there are no other uses of the value.
>>>
>>> Indeed.  Uros, the peep2_reg_dead_p check was added by your patch as svn
>>> revision 211776, git commit e51f8b8fed.  Would you agree that the check is not
>>> necessary for sibcalls as Honza explains?  Would you approve a patch that
>>> removes it in the sibcall peepholes I modify in the patch under discussion?
>>>
>>>> Don't we however need to check that operands[0] is not used by the call_insn as
>>>> parameter of the call?  I.e. something like
>>>>
>>>> void
>>>> test(void (*callback ()))
>>>> {
>>>>    callback(callback);
>>>> }
>>>
>>> You need a pointer-to-pointer-to-function to trigger the peephole.  Something
>>> like this:
>>>
>>>    void foo()
>>>    {
>>>      void (**bar)(void*);
>>>      asm("":"=r"(bar));
>>>      (*bar)(*bar);
>>>    }
>>>
>>>> I think instead of peep2_reg_dead_p we want to check that the parameter is not in
>>>> CALL_INSN_FUNCTION_USAGE of the sibcall..
>>>
>>> Playing with the above testcase I can't induce failure.  It seems today GCC
>>> won't allocate the same register as callee address and one of the arguments.
>>> Do you want me to implement such a check anyway?
>>
>> Hmm, only way I can trigger same register is:
>> void foo()
>>    {
>>      void (**bar)(void*);
>>      asm("":"=r"(bar));
>>      register void (*var)(void *) asm("%eax");
>>      var=*bar;
>>      asm("":"+r"(var));
>>      var(var);
>>    }
>>
>> removing the second asm makes CSE to forward propagae the memory operand
>> to call that makes the call different from the register variable.
>>
>> Still I would check for that, but this is more Uros' area.
>
> This is from [1], and reading this reference, it looks to me that the
> check was introduced due to:
>
> - Adds check that eliminated register is really dead after the call
> (maybe an overkill, but some hard-to-debug problems surfaced due to
> missing liveness checks in the past)
>
> Going down that memory lane, it looks like a safety check for
> something that *might* happen. Looking at the comment, I'd say we can
> remove the check, but we should look for possible fallout.
I'd tend to agree.

jeff