[5/9] ifcvt: Allow constants operands in noce_convert_multiple_sets.

Message ID 20190802151028.15590-6-rdapp@linux.ibm.com
State New
Headers show
Series
  • Improve icvt "convert multiple"
Related show

Commit Message

Robin Dapp Aug. 2, 2019, 3:10 p.m.
This patch checks allows immediate then/else operands for cmovs.
We rely on,emit_conditional_move returning NULL if something unsupported
was generated.

Also, minor refactoring is performed.

--

gcc/ChangeLog:

2018-11-14  Robin Dapp  <rdapp@linux.ibm.com>

	* ifcvt.c (have_const_cmov): New function.
	(noce_convert_multiple_sets): Allow constants if supported.
	(bb_ok_for_noce_convert_multiple_sets): Likewise.
	(check_cond_move_block): Refactor.
---
 gcc/ifcvt.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

-- 
2.17.0

Comments

Richard Sandiford Aug. 6, 2019, 8:29 p.m. | #1
Robin Dapp <rdapp@linux.ibm.com> writes:
> This patch checks allows immediate then/else operands for cmovs.

> We rely on,emit_conditional_move returning NULL if something unsupported

> was generated.


It seems like this is making noce_convert_multiple_sets overlap
a lot with cond_move_process_if_block (although that uses CONSTANT_P
instead of CONST_INT_P).  How do they fit together after this patch,
i.e. which cases is each one meant to handle that the other doesn't?

Thanks,
Richard

> Also, minor refactoring is performed.

>

> --

>

> gcc/ChangeLog:

>

> 2018-11-14  Robin Dapp  <rdapp@linux.ibm.com>

>

> 	* ifcvt.c (have_const_cmov): New function.

> 	(noce_convert_multiple_sets): Allow constants if supported.

> 	(bb_ok_for_noce_convert_multiple_sets): Likewise.

> 	(check_cond_move_block): Refactor.

> ---

>  gcc/ifcvt.c | 21 +++++++++++----------

>  1 file changed, 11 insertions(+), 10 deletions(-)

>

> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c

> index 55205cac153..99716e5f63c 100644

> --- a/gcc/ifcvt.c

> +++ b/gcc/ifcvt.c

> @@ -3214,7 +3214,9 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)

>  	 we'll end up trying to emit r4:HI = cond ? (r1:SI) : (r3:HI).

>  	 Wrap the two cmove operands into subregs if appropriate to prevent

>  	 that.  */

> -      if (GET_MODE (new_val) != GET_MODE (temp))

> +

> +      if (!CONST_INT_P (new_val)

> +         && GET_MODE (new_val) != GET_MODE (temp))

>  	{

>  	  machine_mode src_mode = GET_MODE (new_val);

>  	  machine_mode dst_mode = GET_MODE (temp);

> @@ -3225,7 +3227,8 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)

>  	    }

>  	  new_val = lowpart_subreg (dst_mode, new_val, src_mode);

>  	}

> -      if (GET_MODE (old_val) != GET_MODE (temp))

> +      if (!CONST_INT_P (old_val)

> +         && GET_MODE (old_val) != GET_MODE (temp))

>  	{

>  	  machine_mode src_mode = GET_MODE (old_val);

>  	  machine_mode dst_mode = GET_MODE (temp);

> @@ -3362,9 +3365,9 @@ bb_ok_for_noce_convert_multiple_sets (basic_block test_bb, unsigned *cost)

>        if (!REG_P (dest))

>  	return false;

>  

> -      if (!(REG_P (src)

> -	   || (GET_CODE (src) == SUBREG && REG_P (SUBREG_REG (src))

> -	       && subreg_lowpart_p (src))))

> +      if (!((REG_P (src) || (CONST_INT_P (src)))

> +	    || (GET_CODE (src) == SUBREG && REG_P (SUBREG_REG (src))

> +	      && subreg_lowpart_p (src))))

>  	return false;

>  

>        /* Destination must be appropriate for a conditional write.  */

> @@ -3724,7 +3727,7 @@ check_cond_move_block (basic_block bb,

>      {

>        rtx set, dest, src;

>  

> -      if (!NONDEBUG_INSN_P (insn) || JUMP_P (insn))

> +      if (!active_insn_p (insn))

>  	continue;

>        set = single_set (insn);

>        if (!set)

> @@ -3740,10 +3743,8 @@ check_cond_move_block (basic_block bb,

>        if (!CONSTANT_P (src) && !register_operand (src, VOIDmode))

>  	return FALSE;

>  

> -      if (side_effects_p (src) || side_effects_p (dest))

> -	return FALSE;

> -

> -      if (may_trap_p (src) || may_trap_p (dest))

> +      /* Check for side effects and trapping.  */

> +      if (!noce_operand_ok (src) || !noce_operand_ok (dest))

>  	return FALSE;

>  

>        /* Don't try to handle this if the source register was
Robin Dapp Aug. 8, 2019, 9:13 a.m. | #2
> It seems like this is making noce_convert_multiple_sets overlap

> a lot with cond_move_process_if_block (although that uses CONSTANT_P

> instead of CONST_INT_P).  How do they fit together after this patch,

> i.e. which cases is each one meant to handle that the other doesn't?


IMHO all of icvt is currently one unholy overlap with itself and the
backend where at least I could not decipher within reasonable time which
part is supposed to cover what :) Ok, it might not be that bad...

Currently convert_multiple only handles situations without an else_bb
but that seems a rather artificial limit and I see no reason why it
could not be unified with cond_move_process_if_block at a later time.

Regards
 Robin
Richard Sandiford Oct. 27, 2019, 1:41 p.m. | #3
Coming back to this just in time for it not to be three months later,
sorry...

I still think it would be better to consolidate ifcvt a bit more,
rather than effectively duplicate bits of cond_move_process_if_block
in noce_convert_multiple_sets.  But perhaps it was a historical
mistake to have two separate routines in the first place, and I guess
making noce_convert_multiple_sets handle more cases might allow us
to get rid of cond_move_process_if_block at some point.  So let's
go ahead with this anyway.

Robin Dapp <rdapp@linux.ibm.com> writes:
> This patch checks allows immediate then/else operands for cmovs.

> We rely on,emit_conditional_move returning NULL if something unsupported

> was generated.

>

> Also, minor refactoring is performed.

>

> --

>

> gcc/ChangeLog:

>

> 2018-11-14  Robin Dapp  <rdapp@linux.ibm.com>

>

> 	* ifcvt.c (have_const_cmov): New function.

> 	(noce_convert_multiple_sets): Allow constants if supported.

> 	(bb_ok_for_noce_convert_multiple_sets): Likewise.

> 	(check_cond_move_block): Refactor.

> ---

>  gcc/ifcvt.c | 21 +++++++++++----------

>  1 file changed, 11 insertions(+), 10 deletions(-)

>

> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c

> index 55205cac153..99716e5f63c 100644

> --- a/gcc/ifcvt.c

> +++ b/gcc/ifcvt.c

> @@ -3214,7 +3214,9 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)

>  	 we'll end up trying to emit r4:HI = cond ? (r1:SI) : (r3:HI).

>  	 Wrap the two cmove operands into subregs if appropriate to prevent

>  	 that.  */

> -      if (GET_MODE (new_val) != GET_MODE (temp))

> +

> +      if (!CONST_INT_P (new_val)

> +         && GET_MODE (new_val) != GET_MODE (temp))

>  	{

>  	  machine_mode src_mode = GET_MODE (new_val);

>  	  machine_mode dst_mode = GET_MODE (temp);

> @@ -3225,7 +3227,8 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)

>  	    }

>  	  new_val = lowpart_subreg (dst_mode, new_val, src_mode);

>  	}

> -      if (GET_MODE (old_val) != GET_MODE (temp))

> +      if (!CONST_INT_P (old_val)

> +         && GET_MODE (old_val) != GET_MODE (temp))

>  	{

>  	  machine_mode src_mode = GET_MODE (old_val);

>  	  machine_mode dst_mode = GET_MODE (temp);

> @@ -3362,9 +3365,9 @@ bb_ok_for_noce_convert_multiple_sets (basic_block test_bb, unsigned *cost)

>        if (!REG_P (dest))

>  	return false;

>  

> -      if (!(REG_P (src)

> -	   || (GET_CODE (src) == SUBREG && REG_P (SUBREG_REG (src))

> -	       && subreg_lowpart_p (src))))

> +      if (!((REG_P (src) || (CONST_INT_P (src)))

> +	    || (GET_CODE (src) == SUBREG && REG_P (SUBREG_REG (src))

> +	      && subreg_lowpart_p (src))))

>  	return false;

>  

>        /* Destination must be appropriate for a conditional write.  */


CONSTANT_P (as for check_cond_move_block) would cover more cases than
CONST_INT_P.  No need for brackets around CONST_INT_P (...).

I was tempted to say we should use register_operand too, but I guess
that allows unwanted (subreg (mem...))s (as if there's any other kind).

> @@ -3724,7 +3727,7 @@ check_cond_move_block (basic_block bb,

>      {

>        rtx set, dest, src;

>  

> -      if (!NONDEBUG_INSN_P (insn) || JUMP_P (insn))

> +      if (!active_insn_p (insn))

>  	continue;

>        set = single_set (insn);

>        if (!set)


I assume this is to skip notes, but shouldn't this still include JUMP_P?

It also looks like it should be an independent patch.

> @@ -3740,10 +3743,8 @@ check_cond_move_block (basic_block bb,

>        if (!CONSTANT_P (src) && !register_operand (src, VOIDmode))

>  	return FALSE;

>  

> -      if (side_effects_p (src) || side_effects_p (dest))

> -	return FALSE;

> -

> -      if (may_trap_p (src) || may_trap_p (dest))

> +      /* Check for side effects and trapping.  */

> +      if (!noce_operand_ok (src) || !noce_operand_ok (dest))

>  	return FALSE;

>  

>        /* Don't try to handle this if the source register was


Seems wrong to use something called noce_operand_ok in a routine
that is explicitly handling cases in which conditional execution
is possible.

More importantly, I can't see anywhere that handles the MEM_P case
specially for cond_move_*, so are you sure this is really safe?
Might be better to leave it as-is if this part is just a clean-up.

Thanks,
Richard

Patch

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 55205cac153..99716e5f63c 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -3214,7 +3214,9 @@  noce_convert_multiple_sets (struct noce_if_info *if_info)
 	 we'll end up trying to emit r4:HI = cond ? (r1:SI) : (r3:HI).
 	 Wrap the two cmove operands into subregs if appropriate to prevent
 	 that.  */
-      if (GET_MODE (new_val) != GET_MODE (temp))
+
+      if (!CONST_INT_P (new_val)
+         && GET_MODE (new_val) != GET_MODE (temp))
 	{
 	  machine_mode src_mode = GET_MODE (new_val);
 	  machine_mode dst_mode = GET_MODE (temp);
@@ -3225,7 +3227,8 @@  noce_convert_multiple_sets (struct noce_if_info *if_info)
 	    }
 	  new_val = lowpart_subreg (dst_mode, new_val, src_mode);
 	}
-      if (GET_MODE (old_val) != GET_MODE (temp))
+      if (!CONST_INT_P (old_val)
+         && GET_MODE (old_val) != GET_MODE (temp))
 	{
 	  machine_mode src_mode = GET_MODE (old_val);
 	  machine_mode dst_mode = GET_MODE (temp);
@@ -3362,9 +3365,9 @@  bb_ok_for_noce_convert_multiple_sets (basic_block test_bb, unsigned *cost)
       if (!REG_P (dest))
 	return false;
 
-      if (!(REG_P (src)
-	   || (GET_CODE (src) == SUBREG && REG_P (SUBREG_REG (src))
-	       && subreg_lowpart_p (src))))
+      if (!((REG_P (src) || (CONST_INT_P (src)))
+	    || (GET_CODE (src) == SUBREG && REG_P (SUBREG_REG (src))
+	      && subreg_lowpart_p (src))))
 	return false;
 
       /* Destination must be appropriate for a conditional write.  */
@@ -3724,7 +3727,7 @@  check_cond_move_block (basic_block bb,
     {
       rtx set, dest, src;
 
-      if (!NONDEBUG_INSN_P (insn) || JUMP_P (insn))
+      if (!active_insn_p (insn))
 	continue;
       set = single_set (insn);
       if (!set)
@@ -3740,10 +3743,8 @@  check_cond_move_block (basic_block bb,
       if (!CONSTANT_P (src) && !register_operand (src, VOIDmode))
 	return FALSE;
 
-      if (side_effects_p (src) || side_effects_p (dest))
-	return FALSE;
-
-      if (may_trap_p (src) || may_trap_p (dest))
+      /* Check for side effects and trapping.  */
+      if (!noce_operand_ok (src) || !noce_operand_ok (dest))
 	return FALSE;
 
       /* Don't try to handle this if the source register was