Re: [PATCH] Don't combine across likely spilled hard reg setters (PR rtl-optimization/59477)
On 15 January 2014 21:55, Jakub Jelinek wrote: ... > The patch removes the likely_spilled_retval stuff Joern wrote a few years > ago because I believe this change should obsolete that. But, have tried to > reproduce the ICE using sh-elf target and haven't managed to do so, 4.9 is > quite a different compiler from 2005-ish gcc. You wouldn't see an ICE, but wrong-code. That being said, it sounds like your case 2) should handle this.
Re: [PATCH] Don't combine across likely spilled hard reg setters (PR rtl-optimization/59477)
On Thu, Jan 16, 2014 at 03:41:57AM +, Joern Rennecke wrote:
> On 15 January 2014 21:55, Jakub Jelinek wrote:
> ...
> > The patch removes the likely_spilled_retval stuff Joern wrote a few years
> > ago because I believe this change should obsolete that. But, have tried to
> > reproduce the ICE using sh-elf target and haven't managed to do so, 4.9 is
> > quite a different compiler from 2005-ish gcc.
>
> You wouldn't see an ICE, but wrong-code.
I've mistyped it above, yeah, I was looking with your functions reverted
and without the rest of my patch if combiner would even attempt to combine
something into the second insn (loading fr1, after the fr0 set) and that
didn't happen on the trunk.
> That being said, it sounds like your case 2) should handle this.
Anyway, for all the changed *.o files I've done:
for i in `cat /tmp/M1`; do readelf -Ws $i | awk '($4=="FUNC" &&
$7!="UND"){print $8" "$3}' | sort > /tmp/1; readelf -Ws ../obj877/$i | awk
'($4=="FUNC" && $7!="UND"){print $8" "$3}' | sort > /tmp/2; j=`cat /tmp/1 | wc
-l`; k=`diff -up /tmp/1 /tmp/2 | grep -v '^+++' | grep '^+' | wc -l`; echo $i
$k/$j; done
to see how many functions in those files actually changed because of this
patch and the numbers are below. The numbers are smaller though, because
it only looks at function sizes, there could be code generation change even
in function that has the same size (e.g. if later alignment makes the
change not visible in the size). For x86_64, the sums are that
5384 functions out of the 43856 symbols (in these ~ 8% of all *.o files)
changed function size. That still looks like a big change.
I wonder if instead of not creating the LOG_LINKS we could just somehow taint
them, add a flag to struct insn_link that we need to be careful about
combining across that link, then in combine_instructions or the flags together
from all the used links and if try_combine is called with this
crosses_likely_spilled_setter_p
flag, if recog accepts the result look at all constraints on all operands of the
new insn and if for any constraint anywhere REG_CLASS_FROM_CONSTRAINT is likely
spilled, punt. Thoughts on if this could be reliable?
i686:
gcc/builtins.o 0/201
gcc/gimple-fold.o 1/71
gcc/tree-sra.o 1/124
gcc/tree-ssa-structalias.o 0/164
gcc/tree-vect-stmts.o 0/128
gcc/tree-vrp.o 0/172
gcc/cp/error.o 0/61
i686-pc-linux-gnu/libcilkrts/.libs/cilk-abi-cilk-for.o 0/16
x86_64:
gcc/builtins.o 0/195
gcc/cfgexpand.o 0/112
gcc/cfghooks.o 0/56
gcc/cgraphunit.o 1/30
gcc/df-core.o 1/82
gcc/final.o 2/89
gcc/fold-const.o 1/144
gcc/ggc-common.o 2/45
gcc/gimple-fold.o 0/70
gcc/gimple-pretty-print.o 1/74
gcc/godump.o 3/18
gcc/haifa-sched.o 3/162
gcc/i386.o 1/500
gcc/ipa-inline.o 0/46
gcc/opts-common.o 1/18
gcc/opts.o 1/17
gcc/passes.o 7/120
gcc/reg-stack.o 0/44
gcc/sched-rgn.o 1/86
gcc/sel-sched-ir.o 1/238
gcc/toplev.o 0/27
gcc/tree-eh.o 0/138
gcc/tree-inline.o 1/111
gcc/tree-sra.o 1/121
gcc/tree-ssa-live.o 0/45
gcc/tree-ssa-phiprop.o 0/24
gcc/tree-ssa-sccvn.o 0/119
gcc/tree-ssa-structalias.o 0/165
gcc/tree-vect-stmts.o 1/129
gcc/tree-vrp.o 1/176
gcc/varasm.o 2/196
gcc/ada/ali-util.o 1/111
gcc/ada/errout.o 1/86
gcc/ada/lib-xref.o 1/93
gcc/ada/urealp.o 2/88
gcc/ada/utils.o 0/141
gcc/build/read-rtl.o 2/23
gcc/c/c-decl.o 1/160
gcc/c/c-objc-common.o 1/8
gcc/c-family/c-opts.o 0/20
gcc/c-family/c-pch.o 1/8
gcc/cp/cp-objcp-common.o 1/15
gcc/cp/optimize.o 0/6
gcc/fortran/expr.o 1/72
gcc/fortran/intrinsic.o 0/44
gcc/fortran/scanner.o 1/47
gcc/go/expressions.o 81/765
gcc/go/gogo.o 14/351
gcc/go/gogo-tree.o 5/64
gcc/go/go.o 1/6
gcc/go/import-archive.o 1/24
gcc/go/import.o 1/47
gcc/go/parse.o 4/148
gcc/go/runtime.o 3/11
gcc/go/statements.o 21/402
gcc/go/types.o 31/566
gcc/go/unsafe.o 0/5
x86_64-unknown-linux-gnu/boehm-gc/.libs/finalize.o 2/23
x86_64-unknown-linux-gnu/boehm-gc/.libs/pthread_support.o 3/36
x86_64-unknown-linux-gnu/boehm-gc/.libs/ptr_chck.o 1/9
x86_64-unknown-linux-gnu/libcilkrts/.libs/cilk-abi-cilk-for.o 2/15
x86_64-unknown-linux-gnu/libcilkrts/.libs/cilk_fiber-unix.o 0/15
x86_64-unknown-linux-gnu/libgfortran/.libs/environ.o 2/18
x86_64-unknown-linux-gnu/libgfortran/.libs/file_pos.o 1/4
x86_64-unknown-linux-gnu/libgfortran/.libs/inquire.o 1/2
x86_64-unknown-linux-gnu/libgfortran/.libs/intrinsics.o 2/34
x86_64-unknown-linux-gnu/libgfortran/.libs/list_read.o 1/22
x86_64-unknown-linux-gnu/libgfortran/.libs/open.o 2/4
x86_64-unknown-linux-gnu/libgfortran/.libs/transfer.o 3/49
x86_64-unknown-linux-gnu/libgfortran/.libs/unit.o 1/19
x86_64-unknown-linux-gnu/libgfortran/.libs/unix.o 1/60
x86_64-unknown-linux-gnu/libgo/.libs/bufio.o 1/70
x86_64-unknown-linux-gnu/libgo/.libs/bytes.o 2/87
x86_64-unknown-linux-gnu/libgo/.libs/expvar.o 0/49
x86_64-unknown-linux-gnu/libgo/.libs/flag.o 4/94
x86_64-unknown-linux-gnu/libgo/.libs/fmt.o 8/159
x86_64-unknown-linux-gnu/libgo/.libs/image.o 2/132
x86_64-unknown-linux-gnu/libgo/.libs/io.o 2/44
x86_64-unknown-linux-gnu/libgo/.libs/malloc.o 0/19
x86_64-unknown-linux-gnu/libgo/.libs/net.o 32/533
x86_64-unk
Re: [PATCH] Don't combine across likely spilled hard reg setters (PR rtl-optimization/59477)
On Wed, 15 Jan 2014, Jakub Jelinek wrote:
> Hi!
>
> As discussed in the PR, when combine combines something across a setter
> of a likely spilled non-fixed hard register, we may get RA ICE, as the
> combined insn might need for reload a hard reg that is live at the point
> where combiner has combined the insn.
>
> The following patch attempts to fix that by not creating LOG_LINKS in
> certain cases.
Err - your description makes it sound like a register allocator / reload /
LRA issue but you are papering over it in combine?
Vlad claims that reload or LRA cannot fix up here but surely splitting
the live range of the live-crossing hardreg twice and spilling it
must be possible, no?
> There are two cases which the patch handles separately:
> 1) CALL_INSN and preceeding load_register_parameters sequence
>(from the call insn up to the earliest insn in the sequence that
>sets some likely spilled non-fixed hard register
that's PR59477, right?
> 2) other setters of likely spilled non-fixed hard register
>(loading of return value of a function into the return register(s),
>explicit register vars, ...)
>
> 1) is handled specially, by disallowing LOG_LINKS from before the first
>insn into the sequence, because in between that first insn and
>the CALL_INSN a likely spilled hard register is live. LOG_LINKS from
>before the sequence to after the CALL_INSN or from within the sequence
>to after the call or from within the sequence to within the sequence
>are allowed
> 2) other setters are handled just like a basic block boundary for LOG_LINKS,
>no LOG_LINKS are allowed to cross that boundary
It seems this is very conservative - the time combine runs you
only have pseudos in the insn that will eventually create the
conflict. In fact,
(define_insn "*ashl3_1"
[(set (match_operand:SWI48 0 "nonimmediate_operand" "=rm,r,r")
(ashift:SWI48 (match_operand:SWI48 1 "nonimmediate_operand"
"0,l,rm")
(match_operand:QI 2 "nonmemory_operand"
"c,M,r")))
(clobber (reg:CC FLAGS_REG))]
"ix86_binary_operator_ok (ASHIFT, mode, operands)"
suggests that IRA can happily choose sth else than cx here even though
Vlad claims
"As r90 in inns 29 needs to be in cx and cx is already alive, neither
reload nor LRA can do anything."
Other passes may have exactly the same issue - if live-ranges cannot
be split during IRA then two shift insns with overlapping shift
amount life-ranges will cause exactly the same problem, no?
Jakub says
I don't think this is a RA bug, the problem is I think that combine
changes:
(insn 20 19 21 2 (set (reg:DI 2 cx)
(const_int 0 [0])) pr59477.C:20 62 {*movdi_internal_rex64}
(nil))
-(insn 21 20 22 2 (set (reg:SI 37 r8)
-(subreg:SI (reg:DI 64) 0)) pr59477.C:20 64 {*movsi_internal}
- (expr_list:REG_DEAD (reg:DI 64)
-(nil)))
+(insn 21 20 22 2 (parallel [
+(set (reg:DI 37 r8)
+(ashift:DI (reg:DI 65)
+(subreg:QI (reg:SI 63 [ v ]) 0)))
+(clobber (reg:CC 17 flags))
+]) pr59477.C:20 503 {*ashldi3_1}
+ (expr_list:REG_UNUSED (reg:CC 17 flags)
+(expr_list:REG_DEAD (reg:SI 63 [ v ])
+(expr_list:REG_DEAD (reg:DI 65)
+(nil)
why is there a non-argument setup insn inbetween argument setup
and the call? Having argument register constraints handled like
;; bar (_20, _20, _20, _20);
(insn 21 20 22 (set (reg:SI 2 cx)
(reg:SI 97 [ D.1780 ])) t.c:10 -1
(nil))
(insn 22 21 23 (set (reg:SI 1 dx)
(reg:SI 97 [ D.1780 ])) t.c:10 -1
(nil))
(insn 23 22 24 (set (reg:SI 4 si)
(reg:SI 97 [ D.1780 ])) t.c:10 -1
(nil))
(insn 24 23 25 (set (reg:SI 5 di)
(reg:SI 97 [ D.1780 ])) t.c:10 -1
(nil))
(call_insn 25 24 0 (call (mem:QI (symbol_ref:DI ("bar") [flags 0x41]
) [0 bar S1 A8])
looks less than ideal - we get a live-range split which is
probably good for RA, but ultimatively there shouldn't be separate
insns involving hard regs before reload - if anything gets
between those insns this limits RA / reload freedom. We
should be able to tell the RA that the call needs its inputs in
certain registers - just as we do with asm()s.
So, looking at what combine does:
Trying 18 -> 28:
Successfully matched this instruction:
(set (reg:SI 37 r8)
(subreg:SI (reg:DI 89 [ D.2308 ]) 0))
that's
(insn 18 17 20 2 (parallel [
(set (reg:DI 95)
(ior:DI (reg:DI 93)
(reg:DI 91 [ D.2309 ])))
(clobber (reg:CC 17 flags))
]) t.c:4 421 {*iordi_1}
(expr_list:REG_DEAD (reg:DI 93)
(expr_list:REG_DEAD (reg:DI 91 [ D.2309 ])
(expr_list:REG_UNUSED (reg:CC 17 flags)
(nil)
(insn 28 27 29 2 (set (reg:SI 37 r8)
(subreg:SI (reg:DI 95) 0)) t.c:20 90 {*movsi_internal}
(expr_list:REG_DEAD (reg:DI 95)
(nil)))
Trying 11 -> 28:
Successfully matched this instructio
Re: [PATCH] Don't combine across likely spilled hard reg setters (PR rtl-optimization/59477)
On 16 January 2014 08:26, Jakub Jelinek wrote: > I wonder if instead of not creating the LOG_LINKS we could just somehow taint > them, add a flag to struct insn_link that we need to be careful about > combining across that link, then in combine_instructions or the flags together > from all the used links and if try_combine is called with this > crosses_likely_spilled_setter_p > flag, if recog accepts the result look at all constraints on all operands of > the > new insn and if for any constraint anywhere REG_CLASS_FROM_CONSTRAINT is > likely > spilled, punt. Thoughts on if this could be reliable? You would miss spills from secondary/tertiary reloads. OTOH, as Richard already said, what you do check is at the same time paranoidly conservative. Since the failure mode here is an ICE, I think it would be appropriate to use a target hook to look at the live likely-spilled register and the combined insn, and tell if the combination is safe - and set the default not to worry.
Re: [PATCH] Don't combine across likely spilled hard reg setters (PR rtl-optimization/59477)
> I think the problem is still either a missed feature in LRA/reload > (split live-ranges), a problem in how we represent calls & argument > setup (magic hard-reg uses), or a backend problem (should spill > itself if necessary, via a post-reload splitter or always spill > and undo via a peephole2). > > Of course papering over in combine might be the best at this > stage. So the above was just observations from the less experienced > people in this area. Yes, this is ultimately a RA issue, but an ancient and hard one as far as I understand so papering over it makes sense I think. That being said, while Joern's machinery was IMO acceptable because relatively simple and localized, Jakub's looks more complicated and far-reaching (that's why I suggested to try to extend the existing machinery if possible) so I think that we ought to try something simpler first. -- Eric Botcazou
Re: [PATCH] Don't combine across likely spilled hard reg setters (PR rtl-optimization/59477)
On 1/20/2014, 4:12 AM, Eric Botcazou wrote: I think the problem is still either a missed feature in LRA/reload (split live-ranges), a problem in how we represent calls & argument setup (magic hard-reg uses), or a backend problem (should spill itself if necessary, via a post-reload splitter or always spill and undo via a peephole2). Of course papering over in combine might be the best at this stage. So the above was just observations from the less experienced people in this area. Yes, this is ultimately a RA issue, but an ancient and hard one as far as I understand so papering over it makes sense I think. That being said, while Joern's machinery was IMO acceptable because relatively simple and localized, Jakub's looks more complicated and far-reaching (that's why I suggested to try to extend the existing machinery if possible) so I think that we ought to try something simpler first. I've just started to work on fixing this in LRA. But I am not going to work on fixing this in reload. It would be just wasting enormous amount of time.
Re: [PATCH] Don't combine across likely spilled hard reg setters (PR rtl-optimization/59477)
> I've just started to work on fixing this in LRA. But I am not going to > work on fixing this in reload. It would be just wasting enormous amount > of time. Yes, fixing it in LRA only would be sufficient for PR rtl-optimization/59477, since it's an x86-specific issue. Other architectures aren't as sensitive as x86 in this area, and the existing counter-measures in combine.c are usually sufficient for them. -- Eric Botcazou
