[Bug tree-optimization/102714] New: A volatile-related problem cased by ipa inline pass

[Bug tree-optimization/102714] New: A volatile-related problem cased by ipa inline pass

[duan.db at linux dot alibaba.com]

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

            Bug ID: 102714
           Summary: A volatile-related problem cased by ipa inline pass
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: duan.db at linux dot alibaba.com
  Target Milestone: ---

Created attachment 51592
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51592&action=edit
test.i

Hi, GCC-trunk has a volatile-related problem with -O2 -fno-strict-aliasing. 
The complete test case test.i file is attached. Below are the main caller and
callee code snippets:

caller code snippets:
 restart:
 parent = ((void *)0);
 radix_tree_load_root(root, &node, &maxindex);
 while (radix_tree_is_internal_node(node)) {
 parent = entry_to_node(node);
 if (node == xa_mk_internal(256))
  goto restart;
 if (parent->shift == 0)
  break;
 }

callee code snippets:
static unsigned radix_tree_load_root(const struct xarray *root,
  struct xa_node **nodep, unsigned long *maxindex)
{
 struct xa_node *node =
 ({typeof(root->xa_head) ________p1 = ({(*(const volatile typeof(root->xa_head)
*)&(root->xa_head)); });
    ((typeof(*root->xa_head) *)(________p1));});

 *nodep = node;

 if (__builtin_expect(!!(radix_tree_is_internal_node(node)), 1)) {
  node = entry_to_node(node);
  *maxindex = node_maxindex(node);
  return node->shift + (0 ? 4 : 6);
 }

 *maxindex = 0;
 return 0;
}

The callee function radix_tree_load_root will assign the volatile attribute to
root->xa_head (struct member of one input parameter), so that xa_head will not
be optimized by subsequent passes, like loop-invariant code motion.
However, during the IPA inline pass, GCC will use function
redirect_call_stmt_to_callee to rewrite the call function statement and the
intput parameter. In this process, the volatile attribute of xa_head will be
lost, which will be optimized and makes the goto jump logic crash. 

Wrong Assembly code:

0000000000000000 <__radix_tree_lookup>:
   0:   4c 8b 47 08             mov    0x8(%rdi),%r8
   4:   4c 89 c6                mov    %r8,%rsi
   7:   4c 89 c0                mov    %r8,%rax
   a:   83 e6 03                and    $0x3,%esi
   d:   48 83 e0 fd             and    $0xfffffffffffffffd,%rax
  11:   31 c9                   xor    %ecx,%ecx
  13:   48 83 fe 02             cmp    $0x2,%rsi
  17:   75 18                   jne    31 <__radix_tree_lookup+0x31>
  19:   0f 1f 80 00 00 00 00    nopl   0x0(%rax)
  20:   48 89 c1                mov    %rax,%rcx
  23:   49 81 f8 02 04 00 00    cmp    $0x402,%r8
  2a:   74 e5                   je     11 <__radix_tree_lookup+0x11>  //The
difference between correct assembly and incorrect assembly is the goto jump.
The correct assembly here should be (je 0 <__radix_tree_lookup>), which means
each cycle needs to re-fetch xa_head from the memory.
  2c:   80 38 00                cmpb   $0x0,(%rax)
  2f:   75 ef                   jne    20 <__radix_tree_lookup+0x20>
  31:   48 85 d2                test   %rdx,%rdx
  34:   74 03                   je     39 <__radix_tree_lookup+0x39>
  36:   48 89 0a                mov    %rcx,(%rdx)
  39:   4c 89 c0                mov    %r8,%rax
  3c:   c3                      retq


Like I see,  when IPA uses the class ipa_param_adjustments to analyze and store
the input parameters information of the call statement,  it does not consider
related volatile information, which leads to the loss of information.

And early inline pass does not have this problem, which only exists in IPA
inline pass. If lower the limit of early inline pass, the problem can be
circumvented.
But I am not particularly familiar with this part of GCC code. For how to fix
this bug, I look forward to getting some suggestions. Thanks a lot.

[Bug tree-optimization/102714] A volatile-related problem cased by ipa inline pass

[jefflexu at linux dot alibaba.com]

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

--- Comment #1 from Jeffle Xu <jefflexu at linux dot alibaba.com> ---
Some more detailed information about the effect to the application:

Linux kernel relies 'volatile' to do synchronization. Let's say, there are two
processes accessing one memory, one of them will write the memory, while the
other will read the memory (through 'volatile') in a loop. That is, these two
processes will synchronize with each other with this shared memory.

With this bug, the process reading the memory may always read the old value
(cached in the register) rather than get the new value (from memory), and thus
will get stuck in the infinite loop and finally causes soft lockup.

[Bug tree-optimization/102714] [11/12 Regression] A volatile-related problem cased by ipa inline pass

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
           Priority|P3                          |P2
            Summary|A volatile-related problem  |[11/12 Regression] A
                   |cased by ipa inline pass    |volatile-related problem
                   |                            |cased by ipa inline pass
           Keywords|                            |wrong-code
   Last reconfirmed|                            |2021-10-13
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
                 CC|                            |jamborm at gcc dot gnu.org
     Ever confirmed|0                           |1
      Known to fail|                            |11.2.0
   Target Milestone|---                         |11.3

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed, it seems to be IPA SRA losing the volatile (maybe it should just
exempt itself from touching those).

Summary for node __radix_tree_lookup/6:
...
  Summary for edge __radix_tree_lookup/6->radix_tree_load_root/5:
    return value ignored
    Parameter 0:
      Scalar param sources: 0
      Pointer pass through from the param given above, safe_to_import_accesses:
0
...
Summary for node radix_tree_load_root/5:
  Returns value
  Descriptor for parameter 0:
    param_size_limit: 16, size_reached: 8, by_ref
    * Access to unit offset: 8, unit size: 8, type: void * const,
alias_ptr_type: void * *, certain
...

Evaluating analysis results for radix_tree_load_root/5
  Will remove return value.
  Will split parameter 0
    - component at byte offset 8, size 8

  Created adjustments:
    m_always_copy_start: 3
    IPA adjusted parameters: 0. IPA_PARAM_OP_SPLIT , offset: 8, base_index: 0,
prev_clone_index: 0, type:  <pointer_type 0x7ffff669f5e8>, alias type: 
<pointer_type 0x7ffff656c3f0> prefix: ISRA
                             1. IPA_PARAM_OP_COPY , base_index: 1,
prev_clone_index: 1
                             2. IPA_PARAM_OP_COPY , base_index: 2,
prev_clone_index: 2
    Will SKIP return.
  Created new node radix_tree_load_root.isra/10

and it materializes the load in the caller.  While in this case it might
very well materialize a volatile load generally it will be difficult
to match the exact volatile loads (and their order) so it will be best
to disqualify parameters involved in volatile accesses from any IPA
SRA transform.  Interestingly enough the local analysis with
isra_track_scalar_param_local_uses on the param returns -1 ...

But we continue with

      if (POINTER_TYPE_P (type))
        {
          type = TREE_TYPE (type);

          if (TREE_CODE (type) == FUNCTION_TYPE
              || TREE_CODE (type) == METHOD_TYPE)
            {
              if (dump_file && (dump_flags & TDF_DETAILS))
                fprintf (dump_file, " not a candidate, reference to "
                         "a function\n");
              continue;
            }
          if (TYPE_VOLATILE (type))
            {
              if (dump_file && (dump_flags & TDF_DETAILS))
                fprintf (dump_file, " not a candidate, reference to "
                         "a volatile type\n");

but well - just volatileness of the type doesn't mean anything ...
ptr_parm_has_nonarg_uses then does

      if (gimple_assign_single_p (stmt))
        {
          tree rhs = gimple_assign_rhs1 (stmt);
          while (handled_component_p (rhs))
            rhs = TREE_OPERAND (rhs, 0);
          if (TREE_CODE (rhs) == MEM_REF
              && TREE_OPERAND (rhs, 0) == name
              && integer_zerop (TREE_OPERAND (rhs, 1))
              && types_compatible_p (TREE_TYPE (rhs),
                                     TREE_TYPE (TREE_TYPE (name)))
              && !TREE_THIS_VOLATILE (rhs))
            uses_ok++;

but the TREE_THIS_VOLATILE check is misplaced and should be at the _outermost_
component-ref.  (side-note - it also feels like this misses an alignment
check)

I have a patch.

[Bug tree-optimization/102714] [11/12 Regression] A volatile-related problem cased by ipa inline pass

[cvs-commit at gcc dot gnu.org]

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

--- Comment #3 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:23cd18c60c8188e3d68eda721cdb739199e85e5b

commit r12-4366-g23cd18c60c8188e3d68eda721cdb739199e85e5b
Author: Richard Biener <rguenther@suse.de>
Date:   Wed Oct 13 09:13:36 2021 +0200

    ipa/102714 - IPA SRA eliding volatile

    The following fixes the volatileness check of IPA SRA which was
    looking at the innermost reference when checking TREE_THIS_VOLATILE
    but the reference to check is the outermost one.

    2021-10-13  Richard Biener  <rguenther@suse.de>

            PR ipa/102714
            * ipa-sra.c (ptr_parm_has_nonarg_uses): Fix volatileness
            check.

            * gcc.dg/ipa/pr102714.c: New testcase.

[Bug tree-optimization/102714] [11 Regression] A volatile-related problem cased by ipa inline pass

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[11/12 Regression] A        |[11 Regression] A
                   |volatile-related problem    |volatile-related problem
                   |cased by ipa inline pass    |cased by ipa inline pass
      Known to work|                            |12.0

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed on trunk sofar.  Thanks for reporting.

[Bug tree-optimization/102714] [11 Regression] A volatile-related problem cased by ipa inline pass

[duan.db at linux dot alibaba.com]

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

--- Comment #5 from Bo Duan <duan.db at linux dot alibaba.com> ---
The patch works.  Thanks you.

[Bug tree-optimization/102714] [11 Regression] A volatile-related problem cased by ipa inline pass

[duan.db at linux dot alibaba.com]

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

--- Comment #6 from Bo Duan <duan.db at linux dot alibaba.com> ---
Hello, should we backport this patch to gcc-10?

[Bug tree-optimization/102714] [11 Regression] A volatile-related problem cased by ipa inline pass

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Bo Duan from comment #6)
> Hello, should we backport this patch to gcc-10?

It's scheduled for a backport to GCC 11, I'm not aware that GCC 10 is affected?

[Bug tree-optimization/102714] [10/11 Regression] A volatile-related problem cased by ipa inline pass

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[11 Regression] A           |[10/11 Regression] A
                   |volatile-related problem    |volatile-related problem
                   |cased by ipa inline pass    |cased by ipa inline pass
   Target Milestone|11.3                        |10.4

[Bug tree-optimization/102714] [10/11 Regression] A volatile-related problem cased by ipa inline pass

[duan.db at linux dot alibaba.com]

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

--- Comment #8 from Bo Duan <duan.db at linux dot alibaba.com> ---
(In reply to Richard Biener from comment #7)
> (In reply to Bo Duan from comment #6)
> > Hello, should we backport this patch to gcc-10?
> 
> It's scheduled for a backport to GCC 11, I'm not aware that GCC 10 is
> affected?

At first, I found this problem on gcc10.2,  so GCC 10 also be affected.

[Bug tree-optimization/102714] [10/11 Regression] A volatile-related problem cased by ipa inline pass

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

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

commit r11-9201-gd60e310a4b58c713c204060db439451d90b54c47
Author: Richard Biener <rguenther@suse.de>
Date:   Wed Oct 13 09:13:36 2021 +0200

    ipa/102714 - IPA SRA eliding volatile

    The following fixes the volatileness check of IPA SRA which was
    looking at the innermost reference when checking TREE_THIS_VOLATILE
    but the reference to check is the outermost one.

    2021-10-13  Richard Biener  <rguenther@suse.de>

            PR ipa/102714
            * ipa-sra.c (ptr_parm_has_nonarg_uses): Fix volatileness
            check.

            * gcc.dg/ipa/pr102714.c: New testcase.

    (cherry picked from commit 23cd18c60c8188e3d68eda721cdb739199e85e5b)

[Bug tree-optimization/102714] [10 Regression] A volatile-related problem cased by ipa inline pass

[duan.db at linux dot alibaba.com]

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

--- Comment #10 from Bo Duan <duan.db at linux dot alibaba.com> ---
(In reply to Richard Biener from comment #7)
> (In reply to Bo Duan from comment #6)
> > Hello, should we backport this patch to gcc-10?
> 
> It's scheduled for a backport to GCC 11, I'm not aware that GCC 10 is
> affected?

Hi, I am going to backport this patch to GCC 10, is it ok for you？

[Bug tree-optimization/102714] [10 Regression] A volatile-related problem cased by ipa inline pass

[rguenther at suse dot de]

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

--- Comment #11 from rguenther at suse dot de <rguenther at suse dot de> ---
On Fri, 5 Nov 2021, duan.db at linux dot alibaba.com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102714
> 
> --- Comment #10 from Bo Duan <duan.db at linux dot alibaba.com> ---
> (In reply to Richard Biener from comment #7)
> > (In reply to Bo Duan from comment #6)
> > > Hello, should we backport this patch to gcc-10?
> > 
> > It's scheduled for a backport to GCC 11, I'm not aware that GCC 10 is
> > affected?
> 
> Hi, I am going to backport this patch to GCC 10, is it ok for you？

I will handle this when I do my next round of backports to GCC 10.

[Bug tree-optimization/102714] [10 Regression] A volatile-related problem cased by ipa inline pass

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:8d016315a95cf4d9b72ee8f299e466457ca154b3

commit r10-10262-g8d016315a95cf4d9b72ee8f299e466457ca154b3
Author: Richard Biener <rguenther@suse.de>
Date:   Wed Oct 13 09:13:36 2021 +0200

    ipa/102714 - IPA SRA eliding volatile

    The following fixes the volatileness check of IPA SRA which was
    looking at the innermost reference when checking TREE_THIS_VOLATILE
    but the reference to check is the outermost one.

    2021-10-13  Richard Biener  <rguenther@suse.de>

            PR ipa/102714
            * ipa-sra.c (ptr_parm_has_nonarg_uses): Fix volatileness
            check.

            * gcc.dg/ipa/pr102714.c: New testcase.

    (cherry picked from commit 23cd18c60c8188e3d68eda721cdb739199e85e5b)

[Bug tree-optimization/102714] [10 Regression] A volatile-related problem cased by ipa inline pass

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |10.3.0
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED
      Known to work|                            |10.3.1

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.