[Bug tree-optimization/114769] New: Suspicious code in vect_recog_sad_pattern()

[Bug tree-optimization/114769] New: Suspicious code in vect_recog_sad_pattern()

[fxue at os dot amperecomputing.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114769

            Bug ID: 114769
           Summary: Suspicious code in vect_recog_sad_pattern()
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: fxue at os dot amperecomputing.com
  Target Milestone: ---

This function may contain buggy code, which was introduced due to recent
support to the new ABD pattern. It calls "vect_recog_absolute_difference" to
check ABS/ABSU statement, if succeed, it expects that "vect_unpromoted_value
unprom[2]" is properly set for the two operands. Though, at the final code
snippet, this is missed, so "unprom" could have unwanted values.

  /* Failed to find a widen operation so we check for a regular MINUS_EXPR.  */
  gassign *diff = dyn_cast <gassign *> (STMT_VINFO_STMT (diff_stmt_vinfo));
  if (diff_stmt && diff
      && gimple_assign_rhs_code (diff) == MINUS_EXPR
      && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (abs_oprnd)))
    {
      *diff_stmt = diff;
      *half_type = NULL_TREE;
      // unprom is not set accordingly
      return true;
    }

This execution path would be triggered for an architecture that merely support
dot-product instruction, but w/o "IFN_ABD" and "IFN_VEC_WIDEN_ABD"
instructions.

[Bug tree-optimization/114769] [14 Regression] Suspicious code in vect_recog_sad_pattern()

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114769

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Suspicious code in          |[14 Regression] Suspicious
                   |vect_recog_sad_pattern()    |code in
                   |                            |vect_recog_sad_pattern()
   Target Milestone|---                         |14.0

[Bug tree-optimization/114769] [14 Regression] Suspicious code in vect_recog_sad_pattern() since r14-1832

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114769

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[14 Regression] Suspicious  |[14 Regression] Suspicious
                   |code in                     |code in
                   |vect_recog_sad_pattern()    |vect_recog_sad_pattern()
                   |                            |since r14-1832
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Added in r14-1832-g710b8dec61a73cb

[Bug tree-optimization/114769] [14 Regression] Suspicious code in vect_recog_sad_pattern() since r14-1832

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114769

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2024-04-19
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org

[Bug tree-optimization/114769] [14 Regression] Suspicious code in vect_recog_sad_pattern() since r14-1832

[tnfchris at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114769

--- Comment #2 from Tamar Christina <tnfchris at gcc dot gnu.org> ---
I believe this is safe, but the interface is definitely not the cleanest.

vect_recog_absolute_difference has two callers:

1. vect_recog_sad_pattern where if you return true with unprom not set, then
*half_type will be NULL.  The call to vect_supportable_direct_optab_p will
always reject it since there's no vector mode for NULL.  Note that if looking
at the dump files, the convention in the dump files have always been that we
first indicate that a pattern could possibly be recognize and then check that
it's supported.

This change somewhat incorrectly makes the diagnostic message get printed for
"invalid" patterns.

2. vect_recog_abd_pattern, where if half_type is NULL, it then uses diff_stmt
to set them.

So while the note in the dump file is misleading, the code is safe.  I will
however slightly refactor it to prevent the confusion.

[Bug tree-optimization/114769] [14 Regression] Suspicious code in vect_recog_sad_pattern() since r14-1832

[fxue at os dot amperecomputing.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114769

--- Comment #3 from Feng Xue <fxue at os dot amperecomputing.com> ---
When half_type is null, and call chain would be:

vect_supportable_direct_optab_p (vinfo, sum_type, SAD_EXPR, NULL, ...)
  -> get_vectype_for_scalar_type (vinfo, NULL)
     -> get_related_vectype_for_scalar_type (vinfo, /*scalar_type= */NULL, ...)

       if ((!INTEGRAL_TYPE_P (scalar_type)
           && !POINTER_TYPE_P (scalar_type)
           && !SCALAR_FLOAT_TYPE_P (scalar_type))

Here will be a segfault.

[Bug tree-optimization/114769] [14 Regression] Suspicious code in vect_recog_sad_pattern() since r14-1832

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114769

--- Comment #4 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tamar Christina <tnfchris@gcc.gnu.org>:

https://gcc.gnu.org/g:1216460e7023cd8ec49933866107417c70e933c9

commit r14-10040-g1216460e7023cd8ec49933866107417c70e933c9
Author: Tamar Christina <tamar.christina@arm.com>
Date:   Fri Apr 19 15:22:13 2024 +0100

    middle-end: refactory vect_recog_absolute_difference to simplify flow
[PR114769]

    Hi All,

    As the reporter in PR114769 points out the control flow for the abd
detection
    is hard to follow.  This is because vect_recog_absolute_difference has two
    different ways it can return true.

    1. It can return true when the widening operation is matched, in which case
       unprom is set, half_type is not NULL and diff_stmt is not set.

    2. It can return true when the widening operation is not matched, but the
stmt
       being checked is a minus.  In this case unprom is not set, half_type is
set
       to NULL and diff_stmt is set.  This because to get to diff_stmt you have
to
       dig through the abs statement and any possible promotions.

    This however leads to complicated uses of the function at the call sites as
the
    exact semantic needs to be known to use it safely.

    vect_recog_absolute_difference has two callers:

    1. vect_recog_sad_pattern where if you return true with unprom not set,
then
       *half_type will be NULL.  The call to vect_supportable_direct_optab_p
will
       always reject it since there's no vector mode for NULL.  Note that if
looking
       at the dump files, the convention in the dump files have always been
that we
       first indicate that a pattern could possibly be recognize and then check
that
       it's supported.

       This change somewhat incorrectly makes the diagnostic message get
printed for
       "invalid" patterns.

    2. vect_recog_abd_pattern, where if half_type is NULL, it then uses
diff_stmt to
       set them.

    This refactors the code, it now only has 1 success condition, and diff_stmt
is
    always set to the minus statement in the abs if there is one.

    The function now only returns success if the widening minus is found, in
which
    case unprom and half_type set.

    This then leaves it up to the caller to decide if they want to do anything
with
    diff_stmt.

    Thanks,
    Tamar

    gcc/ChangeLog:

            PR tree-optimization/114769
            * tree-vect-patterns.cc:
            (vect_recog_absolute_difference): Have only one success condition.
            (vect_recog_abd_pattern): Handle further checks if
            vect_recog_absolute_difference fails.

[Bug tree-optimization/114769] [14 Regression] Suspicious code in vect_recog_sad_pattern() since r14-1832

[tnfchris at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114769

Tamar Christina <tnfchris at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #5 from Tamar Christina <tnfchris at gcc dot gnu.org> ---
(In reply to Feng Xue from comment #3)
> When half_type is null, and call chain would be:
> 
> vect_supportable_direct_optab_p (vinfo, sum_type, SAD_EXPR, NULL, ...)
>   -> get_vectype_for_scalar_type (vinfo, NULL)
>      -> get_related_vectype_for_scalar_type (vinfo, /*scalar_type= */NULL,
> ...)
>              
>        if ((!INTEGRAL_TYPE_P (scalar_type)
>            && !POINTER_TYPE_P (scalar_type)
>            && !SCALAR_FLOAT_TYPE_P (scalar_type))
> 
> Here will be a segfault.

Sure, I wasn't able to trigger that even disabling the optab.
In any case this version doesn't get there anymore.

So fixed, thanks for the report!