diff mbox series

expr: catch more `a*bool` while expanding [PR 112935]

Message ID 20231210092125.1535761-1-quic_apinski@quicinc.com
State Accepted
Headers
Series expr: catch more `a*bool` while expanding [PR 112935] |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Andrew Pinski (QUIC) Dec. 10, 2023, 9:21 a.m. UTC 
  After r14-1655-g52c92fb3f40050 (and the other commits
which touch zero_one_valued_p), we end up with a with
`bool * a` but where the bool is an SSA name that might not
have non-zero bits set on it (to 0x1) even though it
does the non-zero bits would be 0x1.
The case of coremarks, it is only phiopt4 which adds the new
ssa name and nothing afterwards updates the nonzero bits on it.
This fixes the regression by using gimple_zero_one_valued_p
rather than tree_nonzero_bits to match the cases where the
SSA_NAME didn't have the non-zero bits set.
gimple_zero_one_valued_p handles one level of cast and also
and an `&`.

Bootstrapped and tested on x86_64-linux-gnu.

gcc/ChangeLog:

	PR middle-end/112935
	* expr.cc (expand_expr_real_2): Use
	gimple_zero_one_valued_p instead of tree_nonzero_bits
	to find boolean defined expressions.

Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
---
 gcc/expr.cc | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Xi Ruoyao Dec. 10, 2023, 9:29 a.m. UTC | #1 
On Sun, 2023-12-10 at 01:21 -0800, Andrew Pinski wrote:
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index 6da51f2aca2..4686cacd22f 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -10209,8 +10209,9 @@ expand_expr_real_2 (sepops ops, rtx target,
> machine_mode tmode,
>        /* Expand X*Y as X&-Y when Y must be zero or one.  */
>        if (SCALAR_INT_MODE_P (mode))
>  	{
> -	  bool bit0_p = tree_nonzero_bits (treeop0) == 1;
> -	  bool bit1_p = tree_nonzero_bits (treeop1) == 1;
> +	  bool gimple_zero_one_valued_p (tree, tree (*)(tree));

Should we declare this in the file scope instead?

> +	  bool bit0_p = gimple_zero_one_valued_p (treeop0, nullptr);
> +	  bool bit1_p = gimple_zero_one_valued_p (treeop1, nullptr);
Richard Biener Dec. 11, 2023, 7:48 a.m. UTC | #2 
On Sun, Dec 10, 2023 at 10:21 AM Andrew Pinski <quic_apinski@quicinc.com> wrote:
>
> After r14-1655-g52c92fb3f40050 (and the other commits
> which touch zero_one_valued_p), we end up with a with
> `bool * a` but where the bool is an SSA name that might not
> have non-zero bits set on it (to 0x1) even though it
> does the non-zero bits would be 0x1.
> The case of coremarks, it is only phiopt4 which adds the new
> ssa name and nothing afterwards updates the nonzero bits on it.
> This fixes the regression by using gimple_zero_one_valued_p
> rather than tree_nonzero_bits to match the cases where the
> SSA_NAME didn't have the non-zero bits set.
> gimple_zero_one_valued_p handles one level of cast and also
> and an `&`.
>
> Bootstrapped and tested on x86_64-linux-gnu.

OK.

I do wonder whether we should do CCP like ops late
to update both alignment and nonzero bits before RTL expansion.
The fold-builtins pass looks spot-on since we remove
assume_aligned & friends there.  I wouldn't exactly fire off CCP
but instead do a RPO walk and just update alignment & nonzero
bits via the bit_value_* helpers and without iteration (using
the SSA name store as lattice).

I'd also say RTL expansion should be re-done creating a
_copy_ of the CFG so we can keep the GIMPLE IL valid
and eventually can use ranger during RTL expansion.

Thanks,
Richard.

> gcc/ChangeLog:
>
>         PR middle-end/112935
>         * expr.cc (expand_expr_real_2): Use
>         gimple_zero_one_valued_p instead of tree_nonzero_bits
>         to find boolean defined expressions.
>
> Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> ---
>  gcc/expr.cc | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index 6da51f2aca2..4686cacd22f 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -10209,8 +10209,9 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
>        /* Expand X*Y as X&-Y when Y must be zero or one.  */
>        if (SCALAR_INT_MODE_P (mode))
>         {
> -         bool bit0_p = tree_nonzero_bits (treeop0) == 1;
> -         bool bit1_p = tree_nonzero_bits (treeop1) == 1;
> +         bool gimple_zero_one_valued_p (tree, tree (*)(tree));
> +         bool bit0_p = gimple_zero_one_valued_p (treeop0, nullptr);
> +         bool bit1_p = gimple_zero_one_valued_p (treeop1, nullptr);
>
>           /* Expand X*Y as X&Y when both X and Y must be zero or one.  */
>           if (bit0_p && bit1_p)
> --
> 2.39.3
>
Richard Biener Dec. 11, 2023, 7:51 a.m. UTC | #3 
On Sun, Dec 10, 2023 at 10:30 AM Xi Ruoyao <xry111@xry111.site> wrote:
>
> On Sun, 2023-12-10 at 01:21 -0800, Andrew Pinski wrote:
> > diff --git a/gcc/expr.cc b/gcc/expr.cc
> > index 6da51f2aca2..4686cacd22f 100644
> > --- a/gcc/expr.cc
> > +++ b/gcc/expr.cc
> > @@ -10209,8 +10209,9 @@ expand_expr_real_2 (sepops ops, rtx target,
> > machine_mode tmode,
> >        /* Expand X*Y as X&-Y when Y must be zero or one.  */
> >        if (SCALAR_INT_MODE_P (mode))
> >       {
> > -       bool bit0_p = tree_nonzero_bits (treeop0) == 1;
> > -       bool bit1_p = tree_nonzero_bits (treeop1) == 1;
> > +       bool gimple_zero_one_valued_p (tree, tree (*)(tree));
>
> Should we declare this in the file scope instead?

An improvement might be to have genmatch create a special header
file with all the match declarations.

Though initially I hoped we could eventually have "inlined" match predicates
to be picked up by genmatch from .cc files, like hidden within comments:

// @match
// (match (...)
//  ...)
// @endmatch

(doesn't solve the declaration part though).

Richard.

> > +       bool bit0_p = gimple_zero_one_valued_p (treeop0, nullptr);
> > +       bool bit1_p = gimple_zero_one_valued_p (treeop1, nullptr);
>
> --
> Xi Ruoyao <xry111@xry111.site>
> School of Aerospace Science and Technology, Xidian University
diff mbox series

Patch

diff --git a/gcc/expr.cc b/gcc/expr.cc
index 6da51f2aca2..4686cacd22f 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -10209,8 +10209,9 @@  expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
       /* Expand X*Y as X&-Y when Y must be zero or one.  */
       if (SCALAR_INT_MODE_P (mode))
 	{
-	  bool bit0_p = tree_nonzero_bits (treeop0) == 1;
-	  bool bit1_p = tree_nonzero_bits (treeop1) == 1;
+	  bool gimple_zero_one_valued_p (tree, tree (*)(tree));
+	  bool bit0_p = gimple_zero_one_valued_p (treeop0, nullptr);
+	  bool bit1_p = gimple_zero_one_valued_p (treeop1, nullptr);
 
 	  /* Expand X*Y as X&Y when both X and Y must be zero or one.  */
 	  if (bit0_p && bit1_p)