Bug 115060 - [15 Regression] Probable an issue around usage of vect_look_through_possible_promotion in tree-vect-patterns.cc
Summary: [15 Regression] Probable an issue around usage of vect_look_through_possible_...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 15.0
: P3 normal
Target Milestone: 15.0
Assignee: Not yet assigned to anyone
URL:
Keywords: testsuite-fail
: 115707 (view as bug list)
Depends on:
Blocks:
 
Reported: 2024-05-13 06:24 UTC by Feng Xue
Modified: 2024-08-06 18:50 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2024-05-13 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Feng Xue 2024-05-13 06:24:40 UTC
The purpose of the function is to peel off type casts to find out root definition for a given value. If patterns are involved, intermediate pattern-defined SSAs would be traversed instead of the originals. A subtlety here is that the root SSA (as the return value) might be the original one, even it has been recognized as a pattern. For example,

   a = (T1) patt_b;
   patt_b = (T2) c;        // b = ...
   patt_c = not-a-cast;    // c = old_seq

Given 'a', the function will return 'c', instead of 'patt_c'. If a caller only does something based on type information of returned SSA, there is no problem. But if caller's pattern recog analysis is to parse definition statement of the SSA, the new pattern statement is bypassed. This tends to be inconsistent with processing logic of the pattern formation pass, which is not quite rational, and seems to be an issue, though does not cause any mistake. Anything that I missed here?

Take one code snippet as example:

vect_recog_mulhs_pattern ()
{
  ...

  vect_unpromoted_value unprom_rshift_input;
  tree rshift_input = vect_look_through_possible_promotion
    (vinfo, gimple_assign_rhs1 (last_stmt), &unprom_rshift_input);

  ...

  /* Get the definition of the shift input.  */
  stmt_vec_info rshift_input_stmt_info
    = vect_get_internal_def (vinfo, rshift_input);
  if (!rshift_input_stmt_info)
    return NULL;
  gassign *rshift_input_stmt
    = dyn_cast <gassign *> (rshift_input_stmt_info->stmt);
  if (!rshift_input_stmt)
    return NULL;

   ...

  if (gimple_assign_rhs_code (rshift_input_stmt) == PLUS_EXPR)  // How about if rshift_input_stmt has a pattern replacement?
    {
      ...
    }
Comment 1 Richard Biener 2024-05-13 10:21:37 UTC
Confirmed.  Probably vect_get_internal_def itself should concern itself with this.
Comment 2 GCC Commits 2024-05-28 14:02:13 UTC
The master branch has been updated by Feng Xue <fxue@gcc.gnu.org>:

https://gcc.gnu.org/g:a3aeff4ce95bd616a2108dc2363d9cbaba53b170

commit r15-863-ga3aeff4ce95bd616a2108dc2363d9cbaba53b170
Author: Feng Xue <fxue@os.amperecomputing.com>
Date:   Thu May 23 15:25:53 2024 +0800

    vect: Use vect representative statement instead of original in patch recog [PR115060]
    
    Some utility functions (such as vect_look_through_possible_promotion) that are
    to find out certain kind of direct or indirect definition SSA for a value, may
    return the original one of the SSA, not its pattern representative SSA, even
    pattern is involved. For example,
    
       a = (T1) patt_b;
       patt_b = (T2) c;        // b = ...
       patt_c = not-a-cast;    // c = ...
    
    Given 'a', the mentioned function will return 'c', instead of 'patt_c'. This
    subtlety would make some pattern recog code that is unaware of it mis-use the
    original instead of the new pattern statement, which is inconsistent wth
    processing logic of the pattern formation pass. This patch corrects the issue
    by forcing another utility function (vect_get_internal_def) return the pattern
    statement information to caller by default.
    
    2024-05-23 Feng Xue <fxue@os.amperecomputing.com>
    
    gcc/
            PR tree-optimization/115060
            * tree-vect-patterns.cc (vect_get_internal_def): Return statement for
            vectorization.
            (vect_widened_op_tree): Call vect_get_internal_def instead of look_def
            to get statement information.
            (vect_recog_widen_abd_pattern): No need to call vect_stmt_to_vectorize.
Comment 3 Feng Xue 2024-06-02 01:41:29 UTC
Linaro reported a regression:
https://linaro.atlassian.net/browse/GNU-1226

Actually, this is not, but exposes a new bug in vect_look_through_possible_promotion. The function fails to figure out root definition if casts involves more than two promotions with sign change as:

long a = (long)b;       // promotion cast
 -> int b = (int)c;     // promotion cast, sign change
   -> unsigned short c = ...;

For this case, the function thinks the 2nd cast has different sign as the 1st, so stop looking through, while "unsigned short -> integer" is a nature sign extension.
Comment 4 Andrew Pinski 2024-06-29 17:31:30 UTC
*** Bug 115707 has been marked as a duplicate of this bug. ***
Comment 5 Feng Xue 2024-08-06 03:43:23 UTC
Fixed.
Comment 6 Thiago Jung Bauermann 2024-08-06 18:50:48 UTC
(In reply to Feng Xue from comment #5)
> Fixed.

I confirmed that I  don't see the failure anymore.
Thank you!