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

[Jeff Law <law@redhat.com>]

On 07/28/2015 04:14 AM, Kyrill Tkachov wrote:
[ Snip ]


> Here's a respin.
> I've reworked bbs_ok_for_cmove_arith to go over BB_A once and record
> the set registers then go over BB_B once and look inside the SET_SRC
> of each insn for those registers. How does this look? Would you like
> me to investigate the data-flow infrastructure approach?
>
> Also, in bb_valid_for_noce_process_p I iterate through the sub-rtxes
> looking for a MEM with the FOR_EACH_SUBRTX machinery.
>
> As I said above, I think moving the insns rather than re-emitting them
> would make the function more convoluted than I think it needs to be.
>
> Bootstrapped and tested on arm, aarch64, x86_64.
>
>
>
> 2015-07-28  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.
>      (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-07-28  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,
>> Kyrill
>>
>>> jeff
>>>
>
>
> ifcvt.patch
>
>
> commit a02417f015b70a0e47170ac7c41a5569392085a5
> Author: Kyrylo Tkachov<kyrylo.tkachov@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 2d97cc5..dae9b41 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -53,6 +53,7 @@
>   #include "tree-pass.h"
>   #include "dbgcnt.h"
>   #include "shrink-wrap.h"
> +#include "rtl-iter.h"
>   #include "ifcvt.h"
Needs to be mentioned in the ChangeLog.

> @@ -1585,6 +1632,116 @@ 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;
> +  bitmap bba_sets = BITMAP_ALLOC (&reg_obstack);
> +
> +  FOR_BB_INSNS (bb_a, a_insn)
> +    {
> +      if (!active_insn_p (a_insn))
> +	continue;
> +
> +      rtx sset_a = single_set (a_insn);
> +
> +      if (!sset_a)
> +	{
> +	  BITMAP_FREE (bba_sets);
> +	  return false;
> +	}
> +
> +      rtx dest_reg = SET_DEST (sset_a);
> +      bitmap_set_bit (bba_sets, REGNO (dest_reg));
> +    }
So there's a tight relationship between the implementation of 
bbs_ok_for_cmove_arith and insn_valid_noce_process_p.  If there wasn't, 
then we'd probably be looking to use note_stores and note_uses.

With that tight relationship, I think the implementations should be 
close together in the file and a comment in insn_valid_noce_process_p 
indicating that if insn_valid_noce_process_p is changed that the 
implementation of bbs_ok_for_cmove_arith may need to change as well.

One things I am a bit worried about is a SET_DEST that is a ZERO_EXTRACT 
or SUBREG.  Those are usually a read and a write.  So you'd need to 
filter them out earlier (noce_operand_ok?) or handle them in the second 
half of bbs_ok_for_cmove_arith.

Jeff