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

[Uros Bizjak <ubizjak at gmail dot com>]

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.

[1] https://gcc.gnu.org/ml/gcc-patches/2014-06/msg01451.html

Uros.