> -----Original Message----- > From: Jiong Wang [mailto:jiong.w...@arm.com] > Sent: Thursday, September 25, 2014 2:13 AM > To: Jeff Law; Zhenqiang Chen > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [PATCH, 2/2] shrink wrap a function with a single loop: split > live_edge > > > On 22/09/14 18:51, Jeff Law wrote: > > On 09/22/14 04:24, Jiong Wang wrote: > >>> Great. Can you send an updated patchkit for review. > >> patch attached. > >> > >> please review, thanks. > >> > >> gcc/ * shrink-wrap.c (move_insn_for_shrink_wrap): Initialize the > >> live-in of new created BB as the intersection of live-in from > >> "old_dest" and live-out from "bb". > > Looks good. However, before committing we need a couple things. > > > > 1. Bootstrap & regression test this variant of the patch. I know you > > tested an earlier one, but please test this one just to be sure. > > > > 2. Testcase. I think you could test for either the reduction in the > > live-in set of the newly created block or that you're shrink wrapping > > one or more functions you didn't previously shrink-wrap. I think it's > > fine if this test is target specific. > > bootstrap ok based on revision 215515. > > while the x86 regression result is interesting. there is no regression on > check-g++, while there is four regression on check-gcc: > > FAIL: gcc.dg/tree-ssa/loadpre10.c (internal compiler error) > FAIL: gcc.dg/tree-ssa/loadpre10.c (test for excess errors) > FAIL: gcc.dg/tree-ssa/pr21417.c (internal compiler error) > FAIL: gcc.dg/tree-ssa/pr21417.c (test for excess errors) > > this is caused by our improving the accuracy of live-in for new created > basic > block. Now we will split > more than one edge for the above two testcase. thus trigger the following > assert in move_insn_for_shrink_wrap: > > /* We should not split more than once for a function. */ > gcc_assert (!(*split_p));
According to the algorithm, it is impossible to split one edge twice. It's possible to split two different edges. But for such cases, the control flow is too complex to perform shrink-wrapping. Anyway, your patch improves the accuracy. You can replace the "gcc_assert" to "return"; or change "split_p" to "splitted_edge" then you can check one edge is not splitted twice. Thanks! -Zhenqiang > take pr21417.c for example, after the patch, two edges will be split, > > before this patch > ================= > .L2: > movq %rdi, %rax > cmpl $142, (%rdi) > jne .L13 > .L4: > all insns sinked here <-- the only split > ... > ... > > popq %rbx > popq %rbp > .L13: > ret > > after this patch > ================ > .L2: > > cmpl $142, (%rdi) > jne .L13 > .L4: > part of insns sinked into here <-- first split > .... > .... > > popq %rbx > popq %rbp > ret > > .L13: > movq %rdi, %rax <-- second split and one instruction moved here > ret > > I don't know why there is a assert to prevent multi split. > > after I remove that assert, pass bootstrap and no regression. > > and for pr21417.c, the multi split more cause one extra "ret" instruction, but > the performance is better, because there > is no need to execute "movq %rdi, %rax" if we go down to L4. > > any comments? > > BTW: I updated the patch with testcase which could not be shrink-wrapped > before this patch. > > thanks. > > -- Jiong > > > > > Jeff > > > >
