Fix store-merging wrong-code issue (PR tree-optimization/86492)

Message ID 20180711221100.GL7166@tucnak
State New
Headers show
Series
  • Fix store-merging wrong-code issue (PR tree-optimization/86492)
Related show

Commit Message

Jakub Jelinek July 11, 2018, 10:11 p.m.
Hi!

The following testcase is a similar issue to PR84503 and the fix is similar,
because coalesce_immediate_stores temporarily sorts the stores on ascending
bitpos and if stores are merged, the merged store is emitted in the location
of the latest of the stores, we need to verify that there is no overlap with
other stores that are originally before that latest store from those we are
considering and overlaps the set of stores we are considering to merge.
In that case we need to punt and not merge (unless we do smarts like prove
overlap between just some of the stores and force reordering).

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk and 8.2?

2018-07-11  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/86492
	* gimple-ssa-store-merging.c
	(imm_store_chain_info::coalesce_immediate_stores): Call
	check_no_overlap even for the merge_overlapping case.  Formatting fix.

	* gcc.c-torture/execute/pr86492.c: New test.


	Jakub

Comments

Richard Biener July 12, 2018, 7:21 a.m. | #1
On Thu, 12 Jul 2018, Jakub Jelinek wrote:

> Hi!
> 
> The following testcase is a similar issue to PR84503 and the fix is similar,
> because coalesce_immediate_stores temporarily sorts the stores on ascending
> bitpos and if stores are merged, the merged store is emitted in the location
> of the latest of the stores, we need to verify that there is no overlap with
> other stores that are originally before that latest store from those we are
> considering and overlaps the set of stores we are considering to merge.
> In that case we need to punt and not merge (unless we do smarts like prove
> overlap between just some of the stores and force reordering).
> 
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk and 8.2?

OK.

Thanks,
Richard.

> 2018-07-11  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/86492
> 	* gimple-ssa-store-merging.c
> 	(imm_store_chain_info::coalesce_immediate_stores): Call
> 	check_no_overlap even for the merge_overlapping case.  Formatting fix.
> 
> 	* gcc.c-torture/execute/pr86492.c: New test.
> 
> --- gcc/gimple-ssa-store-merging.c.jj	2018-06-13 10:05:53.000000000 +0200
> +++ gcc/gimple-ssa-store-merging.c	2018-07-11 19:24:12.084120206 +0200
> @@ -2702,7 +2702,12 @@ imm_store_chain_info::coalesce_immediate
>  	{
>  	  /* Only allow overlapping stores of constants.  */
>  	  if (info->rhs_code == INTEGER_CST
> -	      && merged_store->stores[0]->rhs_code == INTEGER_CST)
> +	      && merged_store->stores[0]->rhs_code == INTEGER_CST
> +	      && check_no_overlap (m_store_info, i, INTEGER_CST,
> +				   MAX (merged_store->last_order, info->order),
> +				   MAX (merged_store->start
> +					+ merged_store->width,
> +					info->bitpos + info->bitsize)))
>  	    {
>  	      merged_store->merge_overlapping (info);
>  	      goto done;
> @@ -2732,10 +2737,8 @@ imm_store_chain_info::coalesce_immediate
>  	      info->ops_swapped_p = true;
>  	    }
>  	  if (check_no_overlap (m_store_info, i, info->rhs_code,
> -				MAX (merged_store->last_order,
> -				     info->order),
> -				MAX (merged_store->start
> -				     + merged_store->width,
> +				MAX (merged_store->last_order, info->order),
> +				MAX (merged_store->start + merged_store->width,
>  				     info->bitpos + info->bitsize)))
>  	    {
>  	      /* Turn MEM_REF into BIT_INSERT_EXPR for bit-field stores.  */
> --- gcc/testsuite/gcc.c-torture/execute/pr86492.c.jj	2018-07-11 19:40:27.760122514 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr86492.c	2018-07-11 19:40:13.460107841 +0200
> @@ -0,0 +1,34 @@
> +/* PR tree-optimization/86492 */
> +
> +union U
> +{
> +  unsigned int r;
> +  struct S
> +  {
> +    unsigned int a:12;
> +    unsigned int b:4;
> +    unsigned int c:16;
> +  } f;
> +};
> +
> +__attribute__((noipa)) unsigned int
> +foo (unsigned int x)
> +{
> +  union U u;
> +  u.r = 0;
> +  u.f.c = x;
> +  u.f.b = 0xe;
> +  return u.r;
> +}
> +
> +int
> +main ()
> +{
> +  union U u;
> +  if (__CHAR_BIT__ * __SIZEOF_INT__ != 32 || sizeof (u.r) != sizeof (u.f))
> +    return 0;
> +  u.r = foo (0x72);
> +  if (u.f.a != 0 || u.f.b != 0xe || u.f.c != 0x72)
> +    __builtin_abort ();
> +  return 0;
> +}
> 
> 	Jakub
> 
>

Patch

--- gcc/gimple-ssa-store-merging.c.jj	2018-06-13 10:05:53.000000000 +0200
+++ gcc/gimple-ssa-store-merging.c	2018-07-11 19:24:12.084120206 +0200
@@ -2702,7 +2702,12 @@  imm_store_chain_info::coalesce_immediate
 	{
 	  /* Only allow overlapping stores of constants.  */
 	  if (info->rhs_code == INTEGER_CST
-	      && merged_store->stores[0]->rhs_code == INTEGER_CST)
+	      && merged_store->stores[0]->rhs_code == INTEGER_CST
+	      && check_no_overlap (m_store_info, i, INTEGER_CST,
+				   MAX (merged_store->last_order, info->order),
+				   MAX (merged_store->start
+					+ merged_store->width,
+					info->bitpos + info->bitsize)))
 	    {
 	      merged_store->merge_overlapping (info);
 	      goto done;
@@ -2732,10 +2737,8 @@  imm_store_chain_info::coalesce_immediate
 	      info->ops_swapped_p = true;
 	    }
 	  if (check_no_overlap (m_store_info, i, info->rhs_code,
-				MAX (merged_store->last_order,
-				     info->order),
-				MAX (merged_store->start
-				     + merged_store->width,
+				MAX (merged_store->last_order, info->order),
+				MAX (merged_store->start + merged_store->width,
 				     info->bitpos + info->bitsize)))
 	    {
 	      /* Turn MEM_REF into BIT_INSERT_EXPR for bit-field stores.  */
--- gcc/testsuite/gcc.c-torture/execute/pr86492.c.jj	2018-07-11 19:40:27.760122514 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr86492.c	2018-07-11 19:40:13.460107841 +0200
@@ -0,0 +1,34 @@ 
+/* PR tree-optimization/86492 */
+
+union U
+{
+  unsigned int r;
+  struct S
+  {
+    unsigned int a:12;
+    unsigned int b:4;
+    unsigned int c:16;
+  } f;
+};
+
+__attribute__((noipa)) unsigned int
+foo (unsigned int x)
+{
+  union U u;
+  u.r = 0;
+  u.f.c = x;
+  u.f.b = 0xe;
+  return u.r;
+}
+
+int
+main ()
+{
+  union U u;
+  if (__CHAR_BIT__ * __SIZEOF_INT__ != 32 || sizeof (u.r) != sizeof (u.f))
+    return 0;
+  u.r = foo (0x72);
+  if (u.f.a != 0 || u.f.b != 0xe || u.f.c != 0x72)
+    __builtin_abort ();
+  return 0;
+}