This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix DCE from removing instructions setting hard frame pointer or other artificially used registers (PR middle-end/32758)


On 8/29/07, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Aug 29, 2007 at 11:19:25AM -0700, Seongbae Park wrote:
> > Can you show me an example of the bad DCE your change prevents ?
>
> Bugzilla has it, but I can repeat it.

Thanks. Sorry I didn't notice the PR number in the subject or ChangeLog :(

> void foo (void *x __attribute__((unused)));
>
> __attribute__((noinline))
> int bar (unsigned int x)
> {
>   void *y = __builtin_alloca (x);
>   foo (y);
>   return 1;
> }
>
> with -O2 -fexceptions with the patch generates:
>
> .bar:
> .LFB2:
>         mflr 0
> .LCFI0:
>         std 31,-8(1)
> .LCFI1:
>         addi 3,3,30
>         std 0,16(1)
> .LCFI2:
>         stdu 1,-128(1)
> .LCFI3:
>         rldicr 3,3,31,32
>         rldicr 3,3,33,59
>         neg 3,3
>         ld 0,0(1)
>         mr 31,1                 # <----- this one
> .LCFI4:                         # <----- and this label
>         stdux 0,1,3
>         addi 3,1,112
>         bl .foo
>         nop
>         ld 1,0(1)
>         li 3,1
>         ld 0,16(1)
>         ld 31,-8(1)
>         mtlr 0
>         blr
>         .long 0
>         .byte 0,0,0,1,128,1,0,0
> .LFE2:
>
> while without the patch removes the above marked mr 31,1
> instruction.  Register 31 is on ppc hard frame pointer, this
> function uses alloca and subtracts non-constant amount from
> the stack pointer (register 1).  Eventhough the hard frame
> pointer isn't visibly used anywhere (except instructions
> which save resp. restore it as it is a call saved register),
> it is used by the EH, so with -fexceptions it must be live
> and have proper value at least on all call instructions
> where the stack is decreased by alloca (bl .foo in this case)
> and e.g. with -fasynchronous-unwind-tables on every single
> instruction when the stack is decreased.
> To make debugging possible it is always needed on all such
> instructions, otherwise the debugger can't find out the
> size of the stack frame.

> > On the face of it, the change looks wrong,
> > as the artificial uses for a block aren't meant to be alive everywhere.
>
> Nope, it has to be alive everywhere, at least on the stdux 0,1,3
> through ld 1,0(1) (the former decreases stack by reg3 bytes,
> the latter loads a value from the stack the former insn saved)
> instructions.
>
> > Beside, it's a per-block set, so the patch as it is
> > (even if making them live everywhere is what we want to do)
> > is fragile - it should really use df_get_artificial_uses(bb_index)
> > to initialize a temporary bitmap.
>
> So why df_simulate_fixup_sets uses it directly then?
> df_get_artificial_uses(bb_index) contains also other regs
> that don't have these properties like hfp, fp, ap
> - EH_USES, EH_RETURN_DATA_REGNO.
>
>         Jakub

Kenny explained your patch to me, and I just read the PR.
I think this patch is ok.

We have lots of cleanup to do regarding artificial uses
(namely, getting rid of it completely)
but that's df maintainer's job to do
(of course, anyone is welcome to clean this part up)
and given what we have now, the patch is fine.

There are other bugs (e.g. ud-chain based dce
would happily delete the same insn) but I'll clean those up later.

Thanks for the fix!
-- 
#pragma ident "Seongbae Park, compiler, http://seongbae.blogspot.com";


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]