On Tue, Nov 28, 2023 at 7:08 AM Andrew Pinski <pins...@gmail.com> wrote:
>
> On Mon, Nov 27, 2023 at 10:04 PM Feng Wang <wangf...@eswincomputing.com>
> wrote:
> >
> > On 2023-11-28 11:06 Andrew Pinski <pins...@gmail.com> wrote:
> > >On Mon, Nov 27, 2023 at 6:56 PM Feng Wang <wangf...@eswincomputing.com>
> > >wrote:
> > >>
> > >> The link of PATCH v1:
> > >> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg326661.html
> > >> This patch add another condition for gimple-cond optimization. Refer to
> > >> the following test case.
> > >> int foo1 (int data, int res)
> > >> {
> > >> res = data & 0xf;
> > >> res |= res << 4;
> > >> if (res < 0x22)
> > >> return 0x22;
> > >> return res;
> > >> }
> > >> with the compilation flag "-O2",
> > >> before this patch the log info of phiopt2 pass is
> > >> <bb 2> [local count: 1073741824]:
> > >> res_5 = data_1(D) & 15;
> > >> _6 = (unsigned int) res_5;
> > >> _7 = _6 * 17;
> > >> res_8 = (int) _7;
> > >> if (_7 <= 33)
> > >> goto <bb 3>; [21.72%]
> > >> else
> > >> goto <bb 4>; [78.28%]
> > >>
> > >> <bb 3> [local count: 233216728]:
> > >>
> > >> <bb 4> [local count: 1073741824]:
> > >> # _9 = PHI <res_8(2), 34(3)>
> > >> return _9;
> > >> after this patch the the log info of phiopt2 pass is
> > >> <bb 2> [local count: 1073741824]:
> > >> res_5 = data_1(D) & 15;
> > >> _6 = (unsigned int) res_5;
> > >> _7 = _6 * 17;
> > >> res_8 = (int) _7;
> > >> _10 = MAX_EXPR <_7, 34>;
> > >> _3 = (int) _10;
> > >> return _3;
> > >> This patch optimizes the phi node to generate "MAX_EXPR".
> > >> The root cause of minmax replacement failure is the type of "_7"
> > >> is unsigned, but the type of const_int "34" is signed. It makes
> > >> types_match (c2_type, from_type) return false. So I add another
> > >> condition to process this scenario.
> > >>
> > >> gcc/ChangeLog:
> > >>
> > >> * match.pd: Add another condition to process type mismatch.
> > >
> > >This should most likely be:
> > > ((cond (cmp (convert1? x) c1) (convert2? x) c2) pattern): Also allow
> > >conversions that only change the sign.
> > >
> > >>
> > >> gcc/testsuite/ChangeLog:
> > >>
> > >> * gcc.dg/tree-ssa/phi-opt-41.c: New test.
> > >> ---
> > >> gcc/match.pd | 5 ++++-
> > >> gcc/testsuite/gcc.dg/tree-ssa/phi-opt-41.c | 24 ++++++++++++++++++++++
> > >> 2 files changed, 28 insertions(+), 1 deletion(-)
> > >> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-41.c
> > >>
> > >> diff --git a/gcc/match.pd b/gcc/match.pd
> > >> index 95225e4ca5f..e864845bfa9 100644
> > >> --- a/gcc/match.pd
> > >> +++ b/gcc/match.pd
> > >> @@ -5419,7 +5419,10 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> > >> && (types_match (c2_type, from_type)
> > >> || (TYPE_PRECISION (c2_type) > TYPE_PRECISION (from_type)
> > >> && (TYPE_UNSIGNED (from_type)
> > >> - || TYPE_SIGN (c2_type) == TYPE_SIGN (from_type)))))
> > >> + || TYPE_SIGN (c2_type) == TYPE_SIGN (from_type)))
> > >> + || (TYPE_UNSIGNED (from_type) != TYPE_UNSIGNED (c2_type)
> > >> + && TYPE_PRECISION (c2_type) == TYPE_PRECISION
> > >> (from_type)
> > >> + && !TYPE_OVERFLOW_WRAPS (c2_type))))
> > >
> > >What is the need for TYPE_OVERFLOW_WRAPS here? Also I think you just
> > >need the check for TYPE_PRECISION instead of the rest.
> > >Maybe instead of types_match here, tree_nop_conversion_p could be used
> > >instead. I am not 100% sure though.
> > >
> > >I also suspect you should add a few other testcases that don't depend
> > >on VRP changing things. Maybe a runtime test too.
> > >
> > >Thanks,
> > >Andrew
> > >
> >
> >
> > I want to make sure the cont_int "c2" won't be overflow,so I use the
> > TYPE_OVERFLOW_WRAPS.
> That is exactly what the `int_fits_type_p (@2, from_type)` check is
> there for in the first place.
I wonder why the types_match is there, if we know the constant fits the type
that should be sufficient if we properly care about the actual sign of
the comparison, that is.
Note we're passing down "mismatched" typed args to minmax_from_comparison, so
we probably have to be careful there, possibly passing down the type
of the comparison
explicitly.
Richard.
> Thanks,
> Andrew
>
> > I checked the code , tree_nop_conversion_p judge the TYPE_PRECISION or
> > TYPE_MODE and doesn't
> > care the unsigned_flag, it should be fine for this scenario, I'm not sure
> > if there's a problem with this
> > modification, I will run the regression to check whether it causes other
> > issues.
>
>
> > Thanks,
> > Feng Wang
> >
> >
> > >> {
> > >> if (cmp != EQ_EXPR)
> > >> code = minmax_from_comparison (cmp, @1, @3, @1, @2);
> > >> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-41.c
> > >> b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-41.c
> > >> new file mode 100644
> > >> index 00000000000..d1101c2f9f7
> > >> --- /dev/null
> > >> +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-41.c
> > >> @@ -0,0 +1,24 @@
> > >> +/* { dg-do compile } */
> > >> +/* { dg-options "-O2 -fdump-tree-phiopt2" } */
> > >> +
> > >> +int foo1 (int data, int res)
> > >> +{
> > >> + res = data & 0xf;
> > >> + res |= res << 4;
> > >> + if (res < 0x22)
> > >> + return 0x22;
> > >> + return res;
> > >> +}
> > >> +
> > >> +int foo2 (int data, int res)
> > >> +{
> > >> + res = data & 0xf;
> > >> + unsigned int r = res;
> > >> + r*=17;
> > >> + res = r;
> > >> + if (r < 0x22)
> > >> + return 0x22;
> > >> + return res;
> > >> +}
> > >> +
> > >> +/* { dg-final { scan-tree-dump-times "MAX_EXPR" 2 "phiopt2" } } */
> > >> \ No newline at end of file
> > >> --
> > >> 2.17.1
> > >>
