On Mon, May 11, 2015 at 8:00 PM, Jan Hubicka <hubi...@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.
