[RFA/RFC] [PR tree-optimization/80635] Optimize some V_C_Es with limited ranges into NOP_EXPRs

[RFA/RFC] [PR tree-optimization/80635] Optimize some V_C_Es with limited ranges into NOP_EXPRs

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 2570 bytes --]


So here's an approach to try and address PR80635.

In this BZ we're getting a false positive uninitialized warning using
std::optional.

As outlined in the BZ this stems from SRA using a VIEW_CONVERT_EXPR  which isn't
handled terribly well by the various optimizers/analysis passes.

We have these key blocks:

;;   basic block 5, loop depth 0
;;    pred:       3
;;                2
  # maybe_a$m_6 = PHI <_5(3), maybe_a$m_4(D)(2)>
  # maybe_a$4_7 = PHI <1(3), 0(2)>
<L0>:
  _8 = maybe_b.live;
  if (_8 != 0)
    goto <bb 6>; [0.00%]
  else
    goto <bb 7>; [0.00%]
;;    succ:       6
;;                7

;;   basic block 6, loop depth 0
;;    pred:       5
  B::~B (&maybe_b.D.2512.m_item);
;;    succ:       7

;;   basic block 7, loop depth 0
;;    pred:       5
;;                6
  maybe_b ={v} {CLOBBER};
  resx 3
;;    succ:       8

;;   basic block 8, loop depth 0
;;    pred:       7
<L1>:
  _9 = VIEW_CONVERT_EXPR<bool>(maybe_a$4_7);
  if (_9 != 0)
    goto <bb 9>; [0.00%]
  else
    goto <bb 10>; [0.00%]
;;    succ:       9
;;                10

Where there is a use of maybe_a$m_6 in block #9.

Of course maybe_a$m_6 only takes the value of maybe_a$m_4(D) when we traverse the
edge 2->5 but in that case maybe_a$4_7 will always have the value zero and thus
we can not reach bb #9..  But the V_C_E gets in the way of the analysis and we
issue the false positive warning.  Martin Jambor has indicated that he doesn't
see a way to avoid the V_C_E from SRA without reintroducing PR52244.

This patch optimizes the V_C_E into a NOP_EXPR by verifying that the V_C_E folds
to a constant value for the min & max values of the range of the input operand
and the result of folding is equal to the original input.  We do some additional
checking beyond just that original value and converted value are equal according
to operand_equal_p.

Eventually the NOP_EXPR also gets removed as well and the conditional in bb8
tests maybe_a$4_7 against 0 directly.

That in turn allows the uninit analysis to determine the use of maybe_a$_m_6 in
block #9 is properly guarded and the false positive is avoided.

The optimization of a V_C_E into a NOP_EXPR via this patch occurs a couple
hundred times during a bootstrap, so this isn't a horribly narrow change just to
fix a false positive warning.

Bootstrapped and regression tested on x86_64.  I've also put it through its paces
in the tester.  The tester's current failures (aarch64, mips, h8) are unrelated
to this patch.


Thoughts?  OK for the trunk?  Alternately I wouldn't lose sleep moving this to
gcc-11.

jeff



[-- Attachment #2: P --]
[-- Type: text/plain, Size: 3266 bytes --]

	PR tree-optimization/90635
	* vr-values.c (simplify_stmt_using_ranges): Simplify V_C_E into
	NOP_EXPR in some cases.

	* g++.dg/warn/Wuninitialized-11.C: New test.


diff --git a/gcc/testsuite/g++.dg/warn/Wuninitialized-11.C b/gcc/testsuite/g++.dg/warn/Wuninitialized-11.C
new file mode 100644
index 00000000000..8e3782264bb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wuninitialized-11.C
@@ -0,0 +1,46 @@
+// { dg-do compile { target c++11 } } 
+// { dg-options "-O2 -Wuninitialized" }
+
+using size_t = decltype(sizeof(1));
+inline void *operator new (size_t, void *p) { return p; }
+template<typename T>
+struct optional
+{
+  optional () : m_dummy (), live (false) {}
+  void emplace () { new (&m_item) T (); live = true; }
+  ~optional () { if (live) m_item.~T (); }
+
+  union
+  {
+    struct {} m_dummy;
+    T m_item;
+  };
+  bool live;
+};
+
+
+extern int get ();
+extern void set (int);
+
+struct A
+{
+  A () : m (get ()) {}
+  ~A () { set (m); }
+
+  int m;
+};
+
+struct B
+{
+  B ();
+  ~B ();
+};
+
+void func ()
+{
+  optional<A> maybe_a;
+  optional<B> maybe_b;
+
+  maybe_a.emplace ();
+  maybe_b.emplace ();
+}
diff --git a/gcc/vr-values.c b/gcc/vr-values.c
index 2e3a0788710..bfd3f4f2cea 100644
--- a/gcc/vr-values.c
+++ b/gcc/vr-values.c
@@ -4308,6 +4308,54 @@ vr_values::simplify_stmt_using_ranges (gimple_stmt_iterator *gsi)
 	    return simplify_bit_ops_using_ranges (gsi, stmt);
 	  break;
 
+
+	case VIEW_CONVERT_EXPR:
+	  /* If we're converting an object with a range which is
+	     directly representable in the final type, then just
+	     turn the VIEW_CONVERT_EXPR into a NOP_EXPR.
+
+	     Various optimizers/analyzers handle the NOP_EXPR
+	     better than VIEW_CONVERT_EXPR.  */
+	  if (TREE_CODE (TREE_OPERAND (rhs1, 0)) == SSA_NAME)
+	    {
+	      const value_range_equiv *vr
+		= get_value_range (TREE_OPERAND (rhs1, 0));
+	      if (vr->kind () == VR_RANGE)
+		{
+		  /* See if the min and max values convert to themselves.  */
+		  tree min = vr->min ();
+		  tree max = vr->min ();
+		  tree min_converted = const_unop (VIEW_CONVERT_EXPR,
+						   TREE_TYPE (lhs),
+						   min);
+		  tree max_converted = const_unop (VIEW_CONVERT_EXPR,
+						   TREE_TYPE (lhs),
+						   max);
+		  if (TREE_CODE (min) == INTEGER_CST
+		      && TREE_CODE (max) == INTEGER_CST
+		      && min_converted
+		      && max_converted
+		      && TREE_CODE (min_converted) == INTEGER_CST
+		      && TREE_CODE (max_converted) == INTEGER_CST
+		      /* These may not be strictly necessary, but to be safe
+			 do not do this optimization if the original or
+			 converted min/max have their sign bit set.  */
+		      && tree_int_cst_sgn (min) != -1
+		      && tree_int_cst_sgn (min_converted) != -1
+		      && tree_int_cst_sgn (max) != -1
+		      && tree_int_cst_sgn (max_converted) != -1
+		      && operand_equal_p (min, min_converted, OEP_ONLY_CONST)
+		      && operand_equal_p (max, max_converted, OEP_ONLY_CONST))
+		    {
+		      gimple_assign_set_rhs_code (stmt, NOP_EXPR);
+		      gimple_assign_set_rhs1 (stmt, TREE_OPERAND (rhs1, 0));
+		      update_stmt (stmt);
+		      return true;
+		    }
+		}
+	    }
+	  break;
+
 	CASE_CONVERT:
 	  if (TREE_CODE (rhs1) == SSA_NAME
 	      && INTEGRAL_TYPE_P (TREE_TYPE (rhs1)))

Re: [RFA/RFC] [PR tree-optimization/80635] Optimize some V_C_Es with limited ranges into NOP_EXPRs

[Richard Biener]

On April 5, 2020 5:25:15 PM GMT+02:00, Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>
>So here's an approach to try and address PR80635.
>
>In this BZ we're getting a false positive uninitialized warning using
>std::optional.
>
>As outlined in the BZ this stems from SRA using a VIEW_CONVERT_EXPR 
>which isn't
>handled terribly well by the various optimizers/analysis passes.
>
>We have these key blocks:
>
>;;   basic block 5, loop depth 0
>;;    pred:       3
>;;                2
>  # maybe_a$m_6 = PHI <_5(3), maybe_a$m_4(D)(2)>
>  # maybe_a$4_7 = PHI <1(3), 0(2)>
><L0>:
>  _8 = maybe_b.live;
>  if (_8 != 0)
>    goto <bb 6>; [0.00%]
>  else
>    goto <bb 7>; [0.00%]
>;;    succ:       6
>;;                7
>
>;;   basic block 6, loop depth 0
>;;    pred:       5
>  B::~B (&maybe_b.D.2512.m_item);
>;;    succ:       7
>
>;;   basic block 7, loop depth 0
>;;    pred:       5
>;;                6
>  maybe_b ={v} {CLOBBER};
>  resx 3
>;;    succ:       8
>
>;;   basic block 8, loop depth 0
>;;    pred:       7
><L1>:
>  _9 = VIEW_CONVERT_EXPR<bool>(maybe_a$4_7);
>  if (_9 != 0)
>    goto <bb 9>; [0.00%]
>  else
>    goto <bb 10>; [0.00%]
>;;    succ:       9
>;;                10
>
>Where there is a use of maybe_a$m_6 in block #9.
>
>Of course maybe_a$m_6 only takes the value of maybe_a$m_4(D) when we
>traverse the
>edge 2->5 but in that case maybe_a$4_7 will always have the value zero
>and thus
>we can not reach bb #9..  But the V_C_E gets in the way of the analysis
>and we
>issue the false positive warning.  Martin Jambor has indicated that he
>doesn't
>see a way to avoid the V_C_E from SRA without reintroducing PR52244.
>
>This patch optimizes the V_C_E into a NOP_EXPR by verifying that the
>V_C_E folds
>to a constant value for the min & max values of the range of the input
>operand
>and the result of folding is equal to the original input.  We do some
>additional
>checking beyond just that original value and converted value are equal
>according
>to operand_equal_p.
>
>Eventually the NOP_EXPR also gets removed as well and the conditional
>in bb8
>tests maybe_a$4_7 against 0 directly.
>
>That in turn allows the uninit analysis to determine the use of
>maybe_a$_m_6 in
>block #9 is properly guarded and the false positive is avoided.
>
>The optimization of a V_C_E into a NOP_EXPR via this patch occurs a
>couple
>hundred times during a bootstrap, so this isn't a horribly narrow
>change just to
>fix a false positive warning.
>
>Bootstrapped and regression tested on x86_64.  I've also put it through
>its paces
>in the tester.  The tester's current failures (aarch64, mips, h8) are
>unrelated
>to this patch.
>
>
>Thoughts?  OK for the trunk?  Alternately I wouldn't lose sleep moving
>this to
>gcc-11.

ISTR Ada uses V_C_E to implement checks on value ranges to types. We need to make sure to not interfere with this. 

Richard. 

>
>jeff

Re: [RFA/RFC] [PR tree-optimization/80635] Optimize some V_C_Es with limited ranges into NOP_EXPRs

[Jeff Law]

On Sun, 2020-04-05 at 20:48 +0200, Richard Biener wrote:
> On April 5, 2020 5:25:15 PM GMT+02:00, Jeff Law via Gcc-patches <
> gcc-patches@gcc.gnu.org> wrote:
> > So here's an approach to try and address PR80635.
> > 
> > In this BZ we're getting a false positive uninitialized warning using
> > std::optional.
> > 
> > As outlined in the BZ this stems from SRA using a VIEW_CONVERT_EXPR 
> > which isn't
> > handled terribly well by the various optimizers/analysis passes.
> > 
> > We have these key blocks:
> > 
> > ;;   basic block 5, loop depth 0
> > ;;    pred:       3
> > ;;                2
> >  # maybe_a$m_6 = PHI <_5(3), maybe_a$m_4(D)(2)>
> >  # maybe_a$4_7 = PHI <1(3), 0(2)>
> > <L0>:
> >  _8 = maybe_b.live;
> >  if (_8 != 0)
> >    goto <bb 6>; [0.00%]
> >  else
> >    goto <bb 7>; [0.00%]
> > ;;    succ:       6
> > ;;                7
> > 
> > ;;   basic block 6, loop depth 0
> > ;;    pred:       5
> >  B::~B (&maybe_b.D.2512.m_item);
> > ;;    succ:       7
> > 
> > ;;   basic block 7, loop depth 0
> > ;;    pred:       5
> > ;;                6
> >  maybe_b ={v} {CLOBBER};
> >  resx 3
> > ;;    succ:       8
> > 
> > ;;   basic block 8, loop depth 0
> > ;;    pred:       7
> > <L1>:
> >  _9 = VIEW_CONVERT_EXPR<bool>(maybe_a$4_7);
> >  if (_9 != 0)
> >    goto <bb 9>; [0.00%]
> >  else
> >    goto <bb 10>; [0.00%]
> > ;;    succ:       9
> > ;;                10
> > 
> > Where there is a use of maybe_a$m_6 in block #9.
> > 
> > Of course maybe_a$m_6 only takes the value of maybe_a$m_4(D) when we
> > traverse the
> > edge 2->5 but in that case maybe_a$4_7 will always have the value zero
> > and thus
> > we can not reach bb #9..  But the V_C_E gets in the way of the analysis
> > and we
> > issue the false positive warning.  Martin Jambor has indicated that he
> > doesn't
> > see a way to avoid the V_C_E from SRA without reintroducing PR52244.
> > 
> > This patch optimizes the V_C_E into a NOP_EXPR by verifying that the
> > V_C_E folds
> > to a constant value for the min & max values of the range of the input
> > operand
> > and the result of folding is equal to the original input.  We do some
> > additional
> > checking beyond just that original value and converted value are equal
> > according
> > to operand_equal_p.
> > 
> > Eventually the NOP_EXPR also gets removed as well and the conditional
> > in bb8
> > tests maybe_a$4_7 against 0 directly.
> > 
> > That in turn allows the uninit analysis to determine the use of
> > maybe_a$_m_6 in
> > block #9 is properly guarded and the false positive is avoided.
> > 
> > The optimization of a V_C_E into a NOP_EXPR via this patch occurs a
> > couple
> > hundred times during a bootstrap, so this isn't a horribly narrow
> > change just to
> > fix a false positive warning.
> > 
> > Bootstrapped and regression tested on x86_64.  I've also put it through
> > its paces
> > in the tester.  The tester's current failures (aarch64, mips, h8) are
> > unrelated
> > to this patch.
> > 
> > 
> > Thoughts?  OK for the trunk?  Alternately I wouldn't lose sleep moving
> > this to
> > gcc-11.
> 
> ISTR Ada uses V_C_E to implement checks on value ranges to types. We need to
> make sure to not interfere with this. 
Eric indicated they use it a lot less often than they used to, but yes we should
try to make sure we're not interfering.

I'll enable Ada for the native targets in the tester and see if that spits out
anything concerning.

Jeff

Re: [RFA/RFC] [PR tree-optimization/80635] Optimize some V_C_Es with limited ranges into NOP_EXPRs

[Eric Botcazou]

> ISTR Ada uses V_C_E to implement checks on value ranges to types. We need to
> make sure to not interfere with this.

Yes, Jeff privately asked some time ago whether this would be problematic and 
I answered that this wouldn't.  Ada no longer uses VIEW_CONVERT_EXPR between 
scalar types, including for checks, for about a decade and I actually said to 
Jeff that the optimizers ought to do the same.

-- 
Eric Botcazou

Re: [RFA/RFC] [PR tree-optimization/80635] Optimize some V_C_Es with limited ranges into NOP_EXPRs

[Eric Botcazou]

> Ada no longer uses VIEW_CONVERT_EXPR between scalar types

... between integral types (it does use it if you explicit request an 
unchecked version between integer and floating-point types for example).

-- 
Eric Botcazou

Re: [RFA/RFC] [PR tree-optimization/80635] Optimize some V_C_Es with limited ranges into NOP_EXPRs

[Richard Biener]

On Sun, Apr 5, 2020 at 5:25 PM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
> So here's an approach to try and address PR80635.
>
> In this BZ we're getting a false positive uninitialized warning using
> std::optional.
>
> As outlined in the BZ this stems from SRA using a VIEW_CONVERT_EXPR  which isn't
> handled terribly well by the various optimizers/analysis passes.
>
> We have these key blocks:
>
> ;;   basic block 5, loop depth 0
> ;;    pred:       3
> ;;                2
>   # maybe_a$m_6 = PHI <_5(3), maybe_a$m_4(D)(2)>
>   # maybe_a$4_7 = PHI <1(3), 0(2)>
> <L0>:
>   _8 = maybe_b.live;
>   if (_8 != 0)
>     goto <bb 6>; [0.00%]
>   else
>     goto <bb 7>; [0.00%]
> ;;    succ:       6
> ;;                7
>
> ;;   basic block 6, loop depth 0
> ;;    pred:       5
>   B::~B (&maybe_b.D.2512.m_item);
> ;;    succ:       7
>
> ;;   basic block 7, loop depth 0
> ;;    pred:       5
> ;;                6
>   maybe_b ={v} {CLOBBER};
>   resx 3
> ;;    succ:       8
>
> ;;   basic block 8, loop depth 0
> ;;    pred:       7
> <L1>:
>   _9 = VIEW_CONVERT_EXPR<bool>(maybe_a$4_7);

So this is a reg-reg copy.  But if you replace it with a NOP_EXPR
it becomes a truncation which is less optimal.

Testcase:

char y;
_Bool x;
void __GIMPLE(ssa) foo()
{
  _Bool _1;
  char _2;

  __BB(2):
  _2 = y;
  _1 = (_Bool)_2;
  x = _1;
  return;
}
void __GIMPLE(ssa) bar()
{
  _Bool _1;
  char _2;

  __BB(2):
  _2 = y;
  _1 = __VIEW_CONVERT <_Bool> (_2);
  x = _1;
  return;
}

where assembly is

foo:
.LFB0:
        .cfi_startproc
        movzbl  y(%rip), %eax
        andl    $1, %eax
        movb    %al, x(%rip)
        ret

vs.

bar:
.LFB1:
        .cfi_startproc
        movzbl  y(%rip), %eax
        movb    %al, x(%rip)
        ret

so the reverse transformation is what should be done ...

Which means other analyses have to improve their handling
of VIEW_CONVERT_EXPR instead.

Richard.

>   if (_9 != 0)
>     goto <bb 9>; [0.00%]
>   else
>     goto <bb 10>; [0.00%]
> ;;    succ:       9
> ;;                10
>
> Where there is a use of maybe_a$m_6 in block #9.
>
> Of course maybe_a$m_6 only takes the value of maybe_a$m_4(D) when we traverse the
> edge 2->5 but in that case maybe_a$4_7 will always have the value zero and thus
> we can not reach bb #9..  But the V_C_E gets in the way of the analysis and we
> issue the false positive warning.  Martin Jambor has indicated that he doesn't
> see a way to avoid the V_C_E from SRA without reintroducing PR52244.
>
> This patch optimizes the V_C_E into a NOP_EXPR by verifying that the V_C_E folds
> to a constant value for the min & max values of the range of the input operand
> and the result of folding is equal to the original input.  We do some additional
> checking beyond just that original value and converted value are equal according
> to operand_equal_p.
>
> Eventually the NOP_EXPR also gets removed as well and the conditional in bb8
> tests maybe_a$4_7 against 0 directly.
>
> That in turn allows the uninit analysis to determine the use of maybe_a$_m_6 in
> block #9 is properly guarded and the false positive is avoided.
>
> The optimization of a V_C_E into a NOP_EXPR via this patch occurs a couple
> hundred times during a bootstrap, so this isn't a horribly narrow change just to
> fix a false positive warning.
>
> Bootstrapped and regression tested on x86_64.  I've also put it through its paces
> in the tester.  The tester's current failures (aarch64, mips, h8) are unrelated
> to this patch.
>
>
> Thoughts?  OK for the trunk?  Alternately I wouldn't lose sleep moving this to
> gcc-11.
>
> jeff
>
>