On Mon, May 5, 2025 at 9:54 PM Andrew Pinski <pins...@gmail.com> wrote:
>
> On Mon, May 5, 2025 at 12:00 AM Richard Biener
> <richard.guent...@gmail.com> wrote:
> >
> > On Mon, May 5, 2025 at 3:45 AM Andrew Pinski <quic_apin...@quicinc.com>
> > wrote:
> > >
> > > Like the patch to phiopt (r15-4033-g1f619fe25925a5f7), this adds rewriting
> > > of VCE to
> > > gimple_with_undefined_signed_overflow/rewrite_to_defined_overflow.
> > > I have not seen a case yet for needing this rewrite but this step is
> > > needed
> > > to use gimple_with_undefined_signed_overflow/rewrite_to_defined_overflow
> > > from
> > > phiopt.
> >
> > So what again was the "undefinedness" here? And why is this relevant
> > for _signed overflow_? Wouldn't it be at least no problem to V_C_E
> > a lower-precision value to a higher precision?
>
> First has nothing to do with `signed integer overflow`, I should have
> renamed the functions after this.
> But the undefinedness is that on the condition VCE is valid since it
> is known to be either 0/1.
> But once you move the VCE to be unconditional, the value coming out of
> the VCE might be out of range for the precision (in this case 0/1).
>
> An example is from PR 116098:
> ```
> <bb 4> [local count: 1073741824]:
> if (value_7 == 0)
> goto <bb 5>; [50.00%]
> else
> goto <bb 6>; [50.00%]
>
> <bb 5> [local count: 536870912]:
> <L2>:
> _6 = VIEW_CONVERT_EXPR<_Bool>(value$8_8);
>
> <bb 6> [local count: 1073741824]:
> # _2 = PHI <_6(5), 1(4)>
> ```
>
> And then we move the VCE from being conditional to being unconditional
> and assume the resulting bool will be well defined.
>
> Note I did found a testcase which shows the issue now on the trunk
> (from GCC 14 onwards) without any new changes, PR 120122. This patch
> will fix that case since we are pulling the VCE out of the loop but
> not rewriting it.
>
> >
> > Maybe we should simply reject V_C_Es of non-mode-precision entities
> > on GIMPLE, much like we reject BIT_FIELD_REFs of those.
>
> I am all for that, SRA is what produces the majority of them.
Can you try how difficult that would be? Since really any bits
outside of 'precision' are undefined, so even 0/1 V_C_E to 32
bits precision doesn't guarantee anything. It's mostly RTL
expansion that does "something" we eventually rely on here.
IIRC the idea of V_C_E of bools is when bools are loaded from
memory as 1 byte value they really have 8 bits of precision
but TYPE_MIN/MAX_VALUE making any value outside of [0,1] UB.
So we can V_C_E that to _Bool (with precision 1), but the
V_C_E back, from _Bool to int, is wrong from the IL side since
there's no constraints on the non existent bits of the _Bool.
So maybe reject just that V_C_E direction, from lower to higher
precision entities?
> >
> > > Bootstrappd and tested on x86_64-linux-gnu.
> > >
> > > gcc/ChangeLog:
> > >
> > > PR tree-optimization/116939
> > > * gimple-fold.cc (gimple_with_undefined_signed_overflow): Return
> > > true
> > > for VCE with integral types.
> > > (rewrite_to_defined_overflow): Handle VCE rewriting to a cast.
> > >
> > > Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> > > ---
> > > gcc/gimple-fold.cc | 30 ++++++++++++++++++++++++++++--
> > > 1 file changed, 28 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> > > index c060ef81a42..a6a7fcbb8c1 100644
> > > --- a/gcc/gimple-fold.cc
> > > +++ b/gcc/gimple-fold.cc
> > > @@ -10602,6 +10602,14 @@ gimple_with_undefined_signed_overflow (gimple
> > > *stmt)
> > > if (!INTEGRAL_TYPE_P (lhs_type)
> > > && !POINTER_TYPE_P (lhs_type))
> > > return false;
> > > + tree rhs = gimple_assign_rhs1 (stmt);
> > > + /* VCE from integral types to another integral types but with
> > > + different precisions need to be changed into casts
> > > + to be well defined. */
> > > + if (gimple_assign_rhs_code (stmt) == VIEW_CONVERT_EXPR
> > > + && INTEGRAL_TYPE_P (TREE_TYPE (TREE_OPERAND (rhs, 0)))
> > > + && is_gimple_val (TREE_OPERAND (rhs, 0)))
> > > + return true;
> > > if (!TYPE_OVERFLOW_UNDEFINED (lhs_type))
> > > return false;
> > > if (!arith_code_with_undefined_signed_overflow
> > > @@ -10630,10 +10638,28 @@ rewrite_to_defined_overflow
> > > (gimple_stmt_iterator *gsi, gimple *stmt,
> > > "overflow ");
> > > print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
> > > }
> > > -
> > > + gimple_seq stmts = NULL;
> > > + /* VCE from integral types to another integral types but with
> > > + different precisions need to be changed into casts
> > > + to be well defined. */
> > > + if (gimple_assign_rhs_code (stmt) == VIEW_CONVERT_EXPR)
> > > + {
> > > + tree rhs = gimple_assign_rhs1 (stmt);
> > > + tree new_rhs = TREE_OPERAND (rhs, 0);
> > > + gcc_assert (is_gimple_val (new_rhs));
> > > + gimple_assign_set_rhs_code (stmt, NOP_EXPR);
> > > + gimple_assign_set_rhs1 (stmt, new_rhs);
> > > + if (in_place)
> > > + update_stmt (stmt);
> > > + else
> > > + {
> > > + gimple_set_modified (stmt, true);
> > > + gimple_seq_add_stmt (&stmts, stmt);
> > > + }
> > > + return stmts;
> > > + }
> > > tree lhs = gimple_assign_lhs (stmt);
> > > tree type = unsigned_type_for (TREE_TYPE (lhs));
> > > - gimple_seq stmts = NULL;
> > > if (gimple_assign_rhs_code (stmt) == ABS_EXPR)
> > > gimple_assign_set_rhs_code (stmt, ABSU_EXPR);
> > > else
> > > --
> > > 2.34.1
> > >
