diff mbox series

Fix PR tree-optimization/86659

Message ID 2153499.tvKfKlq6V1@polaris
State New
Headers show
Series Fix PR tree-optimization/86659 | expand

Commit Message

Eric Botcazou Sept. 28, 2018, 4:59 p.m. UTC 
Hi,

this is a regression introduced by the canonicalization of BIT_FIELD_REF in 
match.pd, which totally disregards the REF_REVERSE_STORAGE_ORDER flag, and 
visible as the failure of gnat.dg/sso/q[23].adb on SPARC 64-bit.  But the 
underlying issue of the missing propagation of the flag during GIMPLE folding 
has probably been latent for quite some time on all active branches.

Tested on x86-64/Linux and SPARC/Solaris, OK for mainline?  And branches?


2018-09-28  Eric Botcazou  <ebotcazou@adacore.com>

	PR tree-optimization/86659
	* gimple-match.h (struct gimple_match_op): Add reverse field.
	(gimple_match_op::set_op): New overloaded method.
	* gimple-match-head.c (maybe_build_generic_op) <BIT_FIELD_REF>: Set
	the REF_REVERSE_STORAGE_ORDER flag on the value.
	(gimple_simplify) <GIMPLE_ASSIGN>: For BIT_FIELD_REF, propagate the
 	REF_REVERSE_STORAGE_ORDER flag and avoid simplifying if it is set.

Comments

Richard Biener Oct. 1, 2018, 8:19 a.m. UTC | #1 
On Fri, Sep 28, 2018 at 7:01 PM Eric Botcazou <ebotcazou@adacore.com> wrote:
>
> Hi,
>
> this is a regression introduced by the canonicalization of BIT_FIELD_REF in
> match.pd, which totally disregards the REF_REVERSE_STORAGE_ORDER flag, and
> visible as the failure of gnat.dg/sso/q[23].adb on SPARC 64-bit.  But the
> underlying issue of the missing propagation of the flag during GIMPLE folding
> has probably been latent for quite some time on all active branches.
>
> Tested on x86-64/Linux and SPARC/Solaris, OK for mainline?  And branches?

@@ -853,7 +857,10 @@ gimple_simplify (gimple *stmt, gimple_ma
                op0 = do_valueize (op0, top_valueize, valueized);
                res_op->set_op (code, type, op0,
                                TREE_OPERAND (rhs1, 1),
-                               TREE_OPERAND (rhs1, 2));
+                               TREE_OPERAND (rhs1, 2),
+                               REF_REVERSE_STORAGE_ORDER (rhs1));
+               if (res_op->reverse)
+                 return valueized;
                return (gimple_resimplify3 (seq, res_op, valueize)
                        || valueized);
              }

so the fix is to simply not optimize here?  Are there correctness issues
with the patterns we have for rev-storage?  But then some cases are let through
via the realpart/imagpart/v_c_e case?  I suppose we should never see
REF_REVERSE_STORAGE_ORDER on refs operating on registers (SSA_NAMEs
or even is_gimple_reg()s)?

Note that I think you need to adjust the GENERIC side as well, for example:

static tree
generic_simplify_BIT_FIELD_REF (location_t ARG_UNUSED (loc), enum
tree_code ARG_UNUSED (code), const tree ARG_UNUSED (type), tree op0,
tree op1, tree op2)
{
...
    case VIEW_CONVERT_EXPR:
      {
        tree o20 = TREE_OPERAND (op0, 0);
        {
/* #line 4690 "/tmp/trunk2/gcc/match.pd" */
          tree captures[3] ATTRIBUTE_UNUSED = { o20, op1, op2 };
          if (__builtin_expect (dump_file && (dump_flags &
TDF_FOLDING), 0)) fprintf (dump_file, "Applying pattern %s:%d,
%s:%d\n", "match.pd", 4690, __FILE__, __LINE__);
          tree res_op0;
          res_op0 = captures[0];
          tree res_op1;
          res_op1 = captures[1];
          tree res_op2;
          res_op2 = captures[2];
          tree res;
          res = fold_build3_loc (loc, BIT_FIELD_REF, type, res_op0,
res_op1, res_op2);

where we lose the reverse-storage attribute as well.  You'd probably
have to cut out
rev-storage refs somewhere in genmatch.c.

Richard.

>
> 2018-09-28  Eric Botcazou  <ebotcazou@adacore.com>
>
>         PR tree-optimization/86659
>         * gimple-match.h (struct gimple_match_op): Add reverse field.
>         (gimple_match_op::set_op): New overloaded method.
>         * gimple-match-head.c (maybe_build_generic_op) <BIT_FIELD_REF>: Set
>         the REF_REVERSE_STORAGE_ORDER flag on the value.
>         (gimple_simplify) <GIMPLE_ASSIGN>: For BIT_FIELD_REF, propagate the
>         REF_REVERSE_STORAGE_ORDER flag and avoid simplifying if it is set.
>
> --
> Eric Botcazou
Eric Botcazou Oct. 2, 2018, 10:07 a.m. UTC | #2 
> so the fix is to simply not optimize here?

Yes, we cannot turn a BIT_FIELD_REF with rev-storage into a VIEW_CONVERT_EXPR.

> Are there correctness issues with the patterns we have for rev-storage?  But
> then some cases are let through via the realpart/imagpart/v_c_e case?  I
> suppose we should never see REF_REVERSE_STORAGE_ORDER on refs operating on
> registers (SSA_NAMEs or even is_gimple_reg()s)?

REF_REVERSE_STORAGE_ORDER is set on BIT_FIELD_REF and MEM_REF only.

> Note that I think you need to adjust the GENERIC side as well, for example
>
> [...]
>
> where we lose the reverse-storage attribute as well.  You'd probably
> have to cut out rev-storage refs somewhere in genmatch.c.

I don't think that's necessary since we never fold top-level BIT_FIELD_REFs at 
the GENERIC level so far.  And given that it's impossible to control the top 
level in match.pd, I'd rather not try something impossible unless rforced to 
do it.  This ought to be controlled directly from fold-const.c, if need be.
Richard Biener Oct. 4, 2018, 12:32 p.m. UTC | #3 
On Tue, Oct 2, 2018 at 12:07 PM Eric Botcazou <ebotcazou@adacore.com> wrote:
>
> > so the fix is to simply not optimize here?
>
> Yes, we cannot turn a BIT_FIELD_REF with rev-storage into a VIEW_CONVERT_EXPR.
>
> > Are there correctness issues with the patterns we have for rev-storage?  But
> > then some cases are let through via the realpart/imagpart/v_c_e case?  I
> > suppose we should never see REF_REVERSE_STORAGE_ORDER on refs operating on
> > registers (SSA_NAMEs or even is_gimple_reg()s)?
>
> REF_REVERSE_STORAGE_ORDER is set on BIT_FIELD_REF and MEM_REF only.

OK.

So I wonder why it is necessary to track 'reverse' in gimple_match_op
at all given we
bail out without optimizing as far as I can see?

That is, isn't the gimple_simplify hunk, adjusted to check
REF_REVERSE_STORAGE_ORDER
instead of res_op->reverse, enough to fix the issue?

> > Note that I think you need to adjust the GENERIC side as well, for example
> >
> > [...]
> >
> > where we lose the reverse-storage attribute as well.  You'd probably
> > have to cut out rev-storage refs somewhere in genmatch.c.
>
> I don't think that's necessary since we never fold top-level BIT_FIELD_REFs at
> the GENERIC level so far.  And given that it's impossible to control the top
> level in match.pd, I'd rather not try something impossible unless rforced to
> do it.  This ought to be controlled directly from fold-const.c, if need be.

OK, fine with me.

Richard.

> --
> Eric Botcazou
Eric Botcazou Oct. 5, 2018, 8:29 a.m. UTC | #4 
> So I wonder why it is necessary to track 'reverse' in gimple_match_op
> at all given we bail out without optimizing as far as I can see?

Because of the valueization?  If it is done, gimple_simplify returns true so 
the result will be synthetized from res_op by means of maybe_build_generic_op.
That's what I was referring to in the opening message by "the underlying issue 
of the missing propagation of the flag during GIMPLE folding".
Richard Biener Oct. 8, 2018, 10:17 a.m. UTC | #5 
On Fri, Oct 5, 2018 at 10:29 AM Eric Botcazou <ebotcazou@adacore.com> wrote:
>
> > So I wonder why it is necessary to track 'reverse' in gimple_match_op
> > at all given we bail out without optimizing as far as I can see?
>
> Because of the valueization?  If it is done, gimple_simplify returns true so
> the result will be synthetized from res_op by means of maybe_build_generic_op.
> That's what I was referring to in the opening message by "the underlying issue
> of the missing propagation of the flag during GIMPLE folding".

Ah, indeed.

Patch is OK - sorry for the confusion.

Richard.

> --
> Eric Botcazou
Eric Botcazou Oct. 9, 2018, 5:16 p.m. UTC | #6 
> 2018-09-28  Eric Botcazou  <ebotcazou@adacore.com>
> 
> 	PR tree-optimization/86659
> 	* gimple-match.h (struct gimple_match_op): Add reverse field.

Jonathan privately remarked that the new member should probably be initialized 
in the constructors (thanks!).  Done thusly, applied on mainline as obvious.

	PR tree-optimization/86659
	* gimple-match.h (gimple_match_op constructors): Initialize reverse.
diff mbox series

Patch

Index: gimple-match.h
===================================================================
--- gimple-match.h	(revision 264686)
+++ gimple-match.h	(working copy)
@@ -98,6 +98,7 @@  struct gimple_match_op
   void set_op (code_helper, tree, tree);
   void set_op (code_helper, tree, tree, tree);
   void set_op (code_helper, tree, tree, tree, tree);
+  void set_op (code_helper, tree, tree, tree, tree, bool);
   void set_op (code_helper, tree, tree, tree, tree, tree);
   void set_op (code_helper, tree, tree, tree, tree, tree, tree);
   void set_value (tree);
@@ -117,6 +118,10 @@  struct gimple_match_op
   /* The type of the result.  */
   tree type;
 
+  /* For a BIT_FIELD_REF, whether the group of bits is stored in reverse order
+     from the target order.  */
+  bool reverse;
+
   /* The number of operands to CODE.  */
   unsigned int num_ops;
 
@@ -243,6 +248,19 @@  gimple_match_op::set_op (code_helper cod
   num_ops = 3;
   ops[0] = op0;
   ops[1] = op1;
+  ops[2] = op2;
+}
+
+inline void
+gimple_match_op::set_op (code_helper code_in, tree type_in,
+			 tree op0, tree op1, tree op2, bool reverse_in)
+{
+  code = code_in;
+  type = type_in;
+  reverse = reverse_in;
+  num_ops = 3;
+  ops[0] = op0;
+  ops[1] = op1;
   ops[2] = op2;
 }
 
Index: gimple-match-head.c
===================================================================
--- gimple-match-head.c	(revision 264686)
+++ gimple-match-head.c	(working copy)
@@ -445,16 +445,20 @@  void
 maybe_build_generic_op (gimple_match_op *res_op)
 {
   tree_code code = (tree_code) res_op->code;
+  tree val;
   switch (code)
     {
     case REALPART_EXPR:
     case IMAGPART_EXPR:
     case VIEW_CONVERT_EXPR:
-      res_op->set_value (build1 (code, res_op->type, res_op->ops[0]));
+      val = build1 (code, res_op->type, res_op->ops[0]);
+      res_op->set_value (val);
       break;
     case BIT_FIELD_REF:
-      res_op->set_value (build3 (code, res_op->type, res_op->ops[0],
-				 res_op->ops[1], res_op->ops[2]));
+      val = build3 (code, res_op->type, res_op->ops[0], res_op->ops[1],
+		    res_op->ops[2]);
+      REF_REVERSE_STORAGE_ORDER (val) = res_op->reverse;
+      res_op->set_value (val);
       break;
     default:;
     }
@@ -853,7 +857,10 @@  gimple_simplify (gimple *stmt, gimple_ma
 		op0 = do_valueize (op0, top_valueize, valueized);
 		res_op->set_op (code, type, op0,
 				TREE_OPERAND (rhs1, 1),
-				TREE_OPERAND (rhs1, 2));
+				TREE_OPERAND (rhs1, 2),
+				REF_REVERSE_STORAGE_ORDER (rhs1));
+		if (res_op->reverse)
+		  return valueized;
 		return (gimple_resimplify3 (seq, res_op, valueize)
 			|| valueized);
 	      }