Hi Bernhard,
On 11/07/15 00:00, Bernhard Reutner-Fischer wrote:
On 10 July 2015 at 14:31, Kyrill Tkachov <kyrylo.tkac...@arm.com> wrote:
Hi all,
This patch makes if-conversion more aggressive when handling code of the
form:
if (test)
x := a //THEN
else
x := b //ELSE
The current code adds the costs of both the THEN and ELSE blocks and proceeds
if they don't
exceed the branch cost. I don't think that's quite a right calculation.
We're going to be executing at least one of the basic blocks anyway.
This patch we instead check the *maximum* of the two blocks against the branch
cost.
This should still catch cases where a high latency instruction appears in one
or both of
the paths.
Shouldn't this maximum also take probability into account? Or maybe
not, would have to think about it tomorrow.
The branch cost that we test against (recorded in if_info earlier in the ifcvt.c
call chain) is either the predictable branch cost or the unpredictable branch
cost, depending on what the predictable_edge_p machinery returned.
I think checking against that should be enough, but it's an easy thing to
experiment
with, so I'm open to arguments in any direction.
$ contrib/check_GNU_style.sh rtl-ifcvt.00.patch
Blocks of 8 spaces should be replaced with tabs.
783:+ return FALSE;
Generally ifcvt.c (resp. the whole tree) could use a
sed -i -e "s/\([[:space:]]\)FALSE/\1false/g" gcc/ifcvt.c
Maybe some of the int predicates could then become bools.
Ok, will go over the style in the patch.
+/* Return iff the registers that the insns in BB_A set do not
+ get used in BB_B. */
Return true iff
I tried to be too formal here ;) https://en.wikipedia.org/wiki/If_and_only_if
I'll use a normal if here.
Did you include go in your testing?
I see:
Unexpected results in this build (new failures)
FAIL: encoding/json
FAIL: go/printer
FAIL: go/scanner
FAIL: html/template
FAIL: log
FAIL: net/http
FAIL: net/http/cgi
FAIL: net/http/cookiejar
FAIL: os
FAIL: text/template
Hmmm. I don't see these failures. I double checked right now and they
appear as PASS in my configuration.
I tested make check-go on x86_64-unknown-linux-gnu configured with
--without-isl --disable-multilib --enable-languages=c,c++,fortran,go.
Are you sure this is not some other issue in your tree?
bbs_ok_for_cmove_arith() looks costly but i guess you looked if
there's some pre-existing cleverness you could have used instead?
I did have a look, but couldn't find any.
The bbs_ok_for_cmove_arith is done after the costs check
so I'd hope that the costs check would already discard
really long basic-blocks.
noce_emit_bb() could use a better comment. Likewise insn_valid_noce_process_p().
insn_rtx_cost() should return an unsigned int, then_cost, else_cost
should thus be unsigned int too.
copy_of_a versus copy_of_insn_b; I'd shorten the latter.
Thanks, good suggestions.
bb_valid_for_noce_process_p() suggests that there is a JOIN_BB param
but there is none?
Also should document the return value (and should not clobber the OUT
params upon failure, no?).
bah, I forgot to update the comment once I modified the function
during development of the patch. I'll fix those.
As for the testcases, it would be nice to have at least a tiny bit for
x86_64, too.
I can put the testcases in gcc.dg and enable them for x86 as well,
but I think a couple of the already pass as is because x86 doesn't
need to do an extra zero_extend inside the basic-block.
PS: no -mbranch-cost and, a tad more seriously, no --param branch-cost either ;)
PPS: attached meant to illustrate comments above. Untested.
Thanks a lot! This is all very helpful.
I'll respin the patch.
Thanks,
Kyrill
cheers,
