On 19/08/15 17:57, Jeff Law wrote:
On 08/12/2015 08:31 AM, Kyrill Tkachov wrote:
2015-08-10 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.
Include rtl-iter.h.
(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.
(contains_mem_rtx_p): Likewise.
(bb_valid_for_noce_process_p): Likewise.
(noce_process_if_block): Allow non-simple basic blocks
where appropriate.
2015-08-11 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 for pinging -- I thought I'd already approved this a few days
ago! But I can't find it in my outbox... So clearly I didn't finish
the final review.
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 1f29646..c33fe24 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -1625,6 +1672,152 @@ noce_try_cmove (struct noce_if_info *if_info)
return FALSE;
}
+/* Helper for bb_valid_for_noce_process_p. Validate that
+ the rtx insn INSN is a single set that does not set
+ the conditional register CC and is in general valid for
+ if-conversion. */
+
+static bool
+insn_valid_noce_process_p (rtx_insn *insn, rtx cc)
+{
+ if (!insn
+ || !NONJUMP_INSN_P (insn)
+ || (cc && set_of (cc, insn)))
+ return false;
+
+ rtx sset = single_set (insn);
+
+ /* Currently support only simple single sets in test_bb. */
+ if (!sset
+ || !noce_operand_ok (SET_DEST (sset))
+ || !noce_operand_ok (SET_SRC (sset)))
+ return false;
+
+ return true;
+}
+
+
+ /* Make sure this is a REG and not some instance
+ of ZERO_EXTRACT or SUBREG or other dangerous stuff. */
+ if (!REG_P (SET_DEST (sset_b)))
+ {
+ BITMAP_FREE (bba_sets);
+ return false;
+ }
BTW, this is overly conservative. You're working with pseudos here, so
you can just treat a ZERO_EXTRACT or SUBREG as a read of the full
underlying register. If this comes up in practice you might consider
handling them as a follow-up patch. I don't think you need to handle
that case immediately though.
I agree, from my testing and investigation this patch strictly
increases the available opportunities, so being conservative here
should not cause any regressions against existing behaviour.
Making it more aggressive can be done as a follow up though, as you said,
I'm not sure how frequently this comes up in practice.
I also can't remember if we discussed what happens if blocks A & B write
to the same register, do we handle that situation correctly?
Hmmm...
The function bb_valid_for_noce_process_p that we call early on
in noce_process_if_block makes sure that the only live reg out
of each basic block is the final common destination ('x' in the
noce_if_info struct definition). Since both basic blocks satisfy
that check I suppose that means that even if A and B do write to
the same intermediate pseudo that should not affect correctness
since the written-to common register would have to be read within
A and B (and nowhere outside A and B), which would cause it to
fail the bbs_ok_for_cmove_arith check that checks that no regs
written in A are read in B (and vice versa).
That's the only issue left in my mind. If we're handling that case
correctly, then this is Ok for the trunk as-is. Else we'll need another
iteration.
Does the above explanation look ok to you?
If so, I'll be away for a week from Monday so I'd rather commit
the patch when I get back so I can deal with any fallout...
Thanks for the reviews!
Kyrill
Thanks,
Jeff
