[PATCH] ifcombine: Fix problem when optimizing 64 bit checks

[PATCH] ifcombine: Fix problem when optimizing 64 bit checks

[Andreas Krebbel]

Hi,

the ifcombine pass produces wrong code when optimizing 64 bit checks.

It emits 1 << x trees and uses the integer_one_node as first operand which
might be too small for 64bit operations.

Converting the integer_one_mode operand to the mode of the original operand
fixes the problem for s390x.

Bootstraps on s390 and i686 still running.
Bootstrap fixed on s390x.

OK for mainline given no testsuite regressions occur?

Bye,

-Andreas-

2007-07-23  Andreas Krebbel  <krebbel1@de.ibm.com>

	* tree-ssa-ifcombine.c (ifcombine_ifandif): Convert the integer_one_mode
	operand to the mode of the original operand.


Index: gcc/tree-ssa-ifcombine.c
===================================================================
*** gcc/tree-ssa-ifcombine.c.orig	2007-07-19 13:56:23.000000000 +0200
--- gcc/tree-ssa-ifcombine.c	2007-07-23 16:41:00.000000000 +0200
*************** ifcombine_ifandif (basic_block inner_con
*** 313,321 ****
        /* Do it.  */
        bsi = bsi_for_stmt (inner_cond);
        t = fold_build2 (LSHIFT_EXPR, TREE_TYPE (name1),
! 		       integer_one_node, bit1);
        t2 = fold_build2 (LSHIFT_EXPR, TREE_TYPE (name1),
! 		        integer_one_node, bit2);
        t = fold_build2 (BIT_IOR_EXPR, TREE_TYPE (name1), t, t2);
        t = force_gimple_operand_bsi (&bsi, t, true, NULL_TREE,
  				    true, BSI_SAME_STMT);
--- 313,323 ----
        /* Do it.  */
        bsi = bsi_for_stmt (inner_cond);
        t = fold_build2 (LSHIFT_EXPR, TREE_TYPE (name1),
! 		       fold_convert (TREE_TYPE (name1), integer_one_node),
! 		       bit1);
        t2 = fold_build2 (LSHIFT_EXPR, TREE_TYPE (name1),
! 		        fold_convert (TREE_TYPE (name1), integer_one_node),
! 			bit2);
        t = fold_build2 (BIT_IOR_EXPR, TREE_TYPE (name1), t, t2);
        t = force_gimple_operand_bsi (&bsi, t, true, NULL_TREE,
  				    true, BSI_SAME_STMT);

Re: [PATCH] ifcombine: Fix problem when optimizing 64 bit checks

[Paolo Bonzini]

> OK for mainline given no testsuite regressions occur?

You should use build_int_cst instead of fold_convert.

Paolo

Re: [PATCH] ifcombine: Fix problem when optimizing 64 bit checks

[Richard Guenther]

On 7/23/07, Paolo Bonzini <bonzini@gnu.org> wrote:
>
> > OK for mainline given no testsuite regressions occur?
>
> You should use build_int_cst instead of fold_convert.

Ok with that suggested change.  Can you add a testcase as well?

Thanks,
Richard.

Re: [PATCH] ifcombine: Fix problem when optimizing 64 bit checks

[Andreas Krebbel]

> Ok with that suggested change.  Can you add a testcase as well?

I'll try to reduce a testcase.

I've committed the following patch after bootstrapping on s390, s390x and i686.

Bye,

-Andreas-

2007-07-24  Andreas Krebbel  <krebbel1@de.ibm.com>

	* tree-ssa-ifcombine.c (ifcombine_ifandif): Use a ONE operand
	with the mode of the original operand instead of
	integer_one_node.


Index: gcc/tree-ssa-ifcombine.c
===================================================================
*** gcc/tree-ssa-ifcombine.c.orig	2007-07-24 08:51:28.000000000 +0200
--- gcc/tree-ssa-ifcombine.c	2007-07-24 08:54:27.000000000 +0200
*************** ifcombine_ifandif (basic_block inner_con
*** 313,321 ****
        /* Do it.  */
        bsi = bsi_for_stmt (inner_cond);
        t = fold_build2 (LSHIFT_EXPR, TREE_TYPE (name1),
! 		       integer_one_node, bit1);
        t2 = fold_build2 (LSHIFT_EXPR, TREE_TYPE (name1),
! 		        integer_one_node, bit2);
        t = fold_build2 (BIT_IOR_EXPR, TREE_TYPE (name1), t, t2);
        t = force_gimple_operand_bsi (&bsi, t, true, NULL_TREE,
  				    true, BSI_SAME_STMT);
--- 313,321 ----
        /* Do it.  */
        bsi = bsi_for_stmt (inner_cond);
        t = fold_build2 (LSHIFT_EXPR, TREE_TYPE (name1),
! 		       build_int_cst (TREE_TYPE (name1), 1), bit1);
        t2 = fold_build2 (LSHIFT_EXPR, TREE_TYPE (name1),
! 		        build_int_cst (TREE_TYPE (name1), 1), bit2);
        t = fold_build2 (BIT_IOR_EXPR, TREE_TYPE (name1), t, t2);
        t = force_gimple_operand_bsi (&bsi, t, true, NULL_TREE,
  				    true, BSI_SAME_STMT);

Re: [PATCH] ifcombine: Fix problem when optimizing 64 bit checks

[Andreas Krebbel]

> Ok with that suggested change.  Can you add a testcase as well?

I've added the following testcase.

Bye,

-Andreas-

2007-07-25  Andreas Krebbel  <krebbel1@de.ibm.com>

	* gcc.dg/20070725-1.c: Testcase for revision 126876 added.


Index: gcc/testsuite/gcc.dg/20070725-1.c
===================================================================
*** /dev/null	1970-01-01 00:00:00.000000000 +0000
--- gcc/testsuite/gcc.dg/20070725-1.c	2007-07-25 13:06:32.000000000 +0200
***************
*** 0 ****
--- 1,63 ----
+ /* This used to fail due to a ifcombine problem wrecking 64bit
+    checks.  Fixed with rev. 126876.  */
+ /* { dg-do run } */
+ /* { dg-options "-O1" } */
+ 
+ struct tree_base
+ {
+   unsigned code:16;
+ 
+   unsigned side_effects_flag:1;
+   unsigned constant_flag:1;
+   unsigned addressable_flag:1;
+   unsigned volatile_flag:1;
+   unsigned readonly_flag:1;
+   unsigned unsigned_flag:1;
+   unsigned asm_written_flag:1;
+   unsigned nowarning_flag:1;
+ 
+   unsigned used_flag:1;
+   unsigned nothrow_flag:1;
+   unsigned static_flag:1;
+   unsigned public_flag:1;
+   unsigned private_flag:1;
+   unsigned protected_flag:1;
+   unsigned deprecated_flag:1;
+   unsigned invariant_flag:1;
+ 
+   unsigned lang_flag_0:1;
+   unsigned lang_flag_1:1;
+   unsigned lang_flag_2:1;
+   unsigned lang_flag_3:1;
+   unsigned lang_flag_4:1;
+   unsigned lang_flag_5:1;
+   unsigned lang_flag_6:1;
+   unsigned visited:1;
+ 
+   unsigned spare:24;
+   unsigned long  a;
+ };
+ 
+ int
+ foo (struct tree_base *rhs)
+ {
+   if (({const struct tree_base* __t = (rhs);  __t;})->readonly_flag
+       && (rhs)->static_flag)
+     return 1;
+ 
+   return 0;
+ }
+ 
+ extern void abort (void);
+ 
+ int
+ main ()
+ {
+   struct tree_base t;
+ 
+   t.readonly_flag = t.static_flag = 0;
+   if (foo (&t))
+     abort ();
+ 
+   return 0;
+ }

Re: [PATCH] ifcombine: Fix problem when optimizing 64 bit checks

[Richard Guenther]

On 7/25/07, Andreas Krebbel <Andreas.Krebbel@de.ibm.com> wrote:
> > Ok with that suggested change.  Can you add a testcase as well?
>
> I've added the following testcase.

Thanks!

Richard.

Re: [PATCH] ifcombine: Fix problem when optimizing 64 bit checks

[Rask Ingemann Lambertsen]

On Wed, Jul 25, 2007 at 01:19:44PM +0200, Andreas Krebbel wrote:
> 
> 2007-07-25  Andreas Krebbel  <krebbel1@de.ibm.com>
> 
> 	* gcc.dg/20070725-1.c: Testcase for revision 126876 added.
> 
> 
> Index: gcc/testsuite/gcc.dg/20070725-1.c
> ===================================================================
> *** /dev/null	1970-01-01 00:00:00.000000000 +0000
> --- gcc/testsuite/gcc.dg/20070725-1.c	2007-07-25 13:06:32.000000000 +0200
> ***************
> *** 0 ****
> --- 1,63 ----
> + /* This used to fail due to a ifcombine problem wrecking 64bit
> +    checks.  Fixed with rev. 126876.  */
> + /* { dg-do run } */
> + /* { dg-options "-O1" } */
> + 
> + struct tree_base
> + {
> +   unsigned code:16;
> + 
[snip]
> + 
> +   unsigned spare:24;
> +   unsigned long  a;
> + };

   The testcase doesn't compile on m32c-unknown-elf:

FAIL: gcc.dg/20070725-1.c (test for excess errors)
Excess errors:
/n/12/rask/src/all/gcc/testsuite/gcc.dg/20070725-1.c:37: error: width of 'spare' exceeds its type

UNRESOLVED: gcc.dg/20070725-1.c compilation failed to produce executable

   Do you think your testcase would still work (i.e. trigger the ifcombine
problem) if the spare field was split into two smaller fields? E.g.

  unsigned spare1:16;
  unsigned spare2:8;

-- 
Rask Ingemann Lambertsen

Re: [PATCH] ifcombine: Fix problem when optimizing 64 bit checks

[Andreas Krebbel]

>    Do you think your testcase would still work (i.e. trigger the ifcombine
> problem) if the spare field was split into two smaller fields? E.g.
> 
>   unsigned spare1:16;
>   unsigned spare2:8;

I've applied the following patch (which does exactly what you
suggested) after verifying that it still fails without the fix.

Bye,

-Andreas-

2007-07-26  Andreas Krebbel  <krebbel1@de.ibm.com>

	* gcc.dg/20070725-1.c: Split the spare field in two to avoid:
	error: width of 'spare' exceeds its type.


Index: gcc/testsuite/gcc.dg/20070725-1.c
===================================================================
*** gcc/testsuite/gcc.dg/20070725-1.c.orig	2007-07-25 13:26:02.000000000 +0200
--- gcc/testsuite/gcc.dg/20070725-1.c	2007-07-26 10:19:02.000000000 +0200
*************** struct tree_base
*** 34,40 ****
    unsigned lang_flag_6:1;
    unsigned visited:1;
  
!   unsigned spare:24;
    unsigned long  a;
  };
  
--- 34,41 ----
    unsigned lang_flag_6:1;
    unsigned visited:1;
  
!   unsigned spare1:16;
!   unsigned spare2:8;
    unsigned long  a;
  };
  

Re: [PATCH] ifcombine: Fix problem when optimizing 64 bit checks

[John David Anglin]

> the ifcombine pass produces wrong code when optimizing 64 bit checks.

The patch seems to fix PR 32828 on hppa64-hp-hpux11.11.  I added
the build_int_cst change.

Thanks,
Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)