Re: [PATCH][RTL-ifcvt] Make non-conditional execution if-conversion more aggressive

[Jeff Law <law at redhat dot com>]

On 08/12/2015 08:31 AM, Kyrill Tkachov wrote:
> 2015-08-10  Kyrylo Tkachov <kyrylo.tkachov@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.tkachov@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 also can't remember if we discussed what happens if blocks A & B write 
to the same register, do we handle that situation correctly?

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.

Thanks,
Jeff