Re: [PATCH] Fix store-merging wrong-code issue (PR tree-optimization/86492)

2018-07-12 Thread Richard Biener
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  
> 
>   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.0 +0200
> +++ gcc/gimple-ssa-store-merging.c2018-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
> 
> 

-- 
Richard Biener 
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 
21284 (AG Nuernberg)


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

2018-07-11 Thread Jakub Jelinek
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  

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.0 +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.jj2018-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