On 07/13/2015 08:03 AM, Kyrill Tkachov wrote:
2015-07-13 Kyrylo Tkachov <kyrylo.tkac...@arm.com>
* ifcvt.c (struct noce_if_info): Add then_simple, else_simple,
then_cost, else_cost fields. Change branch_cost field to unsigned
int.
(end_ifcvt_sequence): Call set_used_flags on each insn in the
sequence.
(noce_simple_bbs): New function.
(noce_try_move): Bail if basic blocks are not simple.
(noce_try_store_flag): Likewise.
(noce_try_store_flag_constants): Likewise.
(noce_try_addcc): Likewise.
(noce_try_store_flag_mask): Likewise.
(noce_try_cmove): Likewise.
(noce_try_minmax): Likewise.
(noce_try_abs): Likewise.
(noce_try_sign_mask): Likewise.
(noce_try_bitop): Likewise.
(bbs_ok_for_cmove_arith): New function.
(noce_emit_all_but_last): Likewise.
(noce_emit_insn): Likewise.
(noce_emit_bb): Likewise.
(noce_try_cmove_arith): Handle non-simple basic blocks.
(insn_valid_noce_process_p): New function.
(bb_valid_for_noce_process_p): Likewise.
(noce_process_if_block): Allow non-simple basic blocks
where appropriate.
2015-07-13 Kyrylo Tkachov <kyrylo.tkac...@arm.com>
* gcc.dg/ifcvt-1.c: New test.
* gcc.dg/ifcvt-2.c: Likewise.
* gcc.dg/ifcvt-3.c: Likewise.
Thanks,
Kyrill
cheers,
ifcvt.patch
commit bc62987a2fa3d9dc3de5a1ed8003a745340255bd
Author: Kyrylo Tkachov<kyrylo.tkac...@arm.com>
Date: Wed Jul 8 15:45:04 2015 +0100
[PATCH][ifcvt] Make non-conditional execution if-conversion more aggressive
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 31849ee..2f0a228 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -1584,6 +1630,96 @@ noce_try_cmove (struct noce_if_info *if_info)
return FALSE;
}
+/* Return true iff the registers that the insns in BB_A set do not
+ get used in BB_B. */
+
+static bool
+bbs_ok_for_cmove_arith (basic_block bb_a, basic_block bb_b)
+{
+ rtx_insn *a_insn;
+ FOR_BB_INSNS (bb_a, a_insn)
+ {
+ if (!active_insn_p (a_insn))
+ continue;
+
+ rtx sset_a = single_set (a_insn);
+
+ if (!sset_a)
+ return false;
+
+ rtx dest_reg = SET_DEST (sset_a);
+ rtx_insn *b_insn;
+
+ FOR_BB_INSNS (bb_b, b_insn)
+ {
+ if (!active_insn_p (b_insn))
+ continue;
+
+ rtx sset_b = single_set (b_insn);
+
+ if (!sset_b)
+ return false;
+
+ if (reg_referenced_p (dest_reg, sset_b))
+ return false;
+ }
+ }
+
+ return true;
+}
Doesn't this have the potential to get very expensive since you end up
looking at every insn in BB_B for every insn in BB_A?
Wouldn't it be better to walk BB_A, gathering the set of all the
registers modified, then do a single walk through BB testing for uses of
those registers?
Don't you have to be careful with MEMs in both blocks?
+
+/* Emit copies of all the active instructions in BB except the last.
+ This is a helper for noce_try_cmove_arith. */
+
+static void
+noce_emit_all_but_last (basic_block bb)
+{
+ rtx_insn *last = last_active_insn (bb, FALSE);
+ rtx_insn *insn;
+ FOR_BB_INSNS (bb, insn)
+ {
+ if (insn != last && active_insn_p (insn))
+ {
+ rtx_insn *to_emit = as_a <rtx_insn *> (copy_rtx (insn));
+
+ emit_insn (PATTERN (to_emit));
+ }
+ }
+}
Won't this create invalid RTL sharing? RTL has strict rules about nodes
can and can not be shared and I'm pretty sure this blindly shares
everything.
Now we may get away with that because you're going to delete all the
insns from BB. But that begs the question why not just move the insns
from BB to their new location rather than re-emiting them?
+
+/* Helper for noce_try_cmove_arith. Emit the pattern TO_EMIT and return
+ the resulting insn or NULL if it's not a valid insn. */
+
+static rtx_insn *
+noce_emit_insn (rtx to_emit)
+{
+ gcc_assert (to_emit);
+ rtx_insn *insn = emit_insn (to_emit);
+
+ if (recog_memoized (insn) < 0)
+ return NULL;
+
+ return insn;
+}
+
+/* Helper for noce_try_cmove_arith. Emit a copy of the insns up to
+ and including the penultimate one in BB if it is not simple
+ (as indicated by SIMPLE). Then emit LAST_INSN as the last
+ insn in the block. The reason for that is that LAST_INSN may
+ have been modified by the preparation in noce_try_cmove_arith. */
+
+static bool
+noce_emit_bb (rtx last_insn, basic_block bb, bool simple)
+{
+ if (bb && !simple)
+ noce_emit_all_but_last (bb);
+
+ if (last_insn && !noce_emit_insn (last_insn))
+ return false;
+
+ return true;
+}
Under what conditions can noce_emit_insn fail and what happens to the
insn stream if it does? It seems to me like the insn stream would be
bogus and we should stop compilation. Which argues that rather than
returning a bool, we should just assert that the insn is memoized and
remove the check in noce_emit_bb.
Or is it the case that we're emitting onto a sequence that we can just
throw away in the event of a failure?
What is the meaning of the return value from noce_emit_bb?
+
+ /* We're going to execute one of the basic blocks anyway, so
+ bail out if the most expensive of the two blocks is unacceptable. */
+ if (MAX (then_cost, else_cost) > COSTS_N_INSNS (if_info->branch_cost))
+ return FALSE;
I believe you or someone else mentioned that this may not be the right
heuristic since you're paying for one arm regardless. But we can refine
as necessary.
+ FOR_BB_INSNS (test_bb, insn)
+ {
+ if (insn != last_insn)
+ {
+ if (!active_insn_p (insn))
+ continue;
+
+ if (!insn_valid_noce_process_p (insn, cc))
+ goto free_bitmap_and_fail;
+
+ rtx sset = single_set (insn);
+ gcc_assert (sset);
+
+ if (MEM_P (SET_SRC (sset)) || MEM_P (SET_DEST (sset)))
+ goto free_bitmap_and_fail;
So don't you need to look deeper into SET_SRC to see if it's a mem? It
might be something like (plus (reg) (mem) no?
Jeff
