[Bug middle-end/107617] New: SCC-VN with len_store and big endian

[Bug middle-end/107617] New: SCC-VN with len_store and big endian

[rdapp at gcc dot gnu.org]

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

            Bug ID: 107617
           Summary: SCC-VN with len_store and big endian
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rdapp at gcc dot gnu.org
                CC: richard.guenther at gmail dot com
  Target Milestone: ---
            Target: s

Created attachment 53871
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53871&action=edit
s390 patch for len_load/len_store

Hi,

Richard and I already quickly discussed this on the mailing list but I didn't
manage to progress analyzing as I was tied up with other things.  Figured I
open a bug for tracking purposes and the possibility to maybe fix it in a later
stage.

I'm evaluating len_load/len_store support on s390 via the attached patch and
seeing a FAIL in

testsuite/gfortran.dg/power_3.f90

built with -march=z16 -O3 --param vect-partial-vector-usage=1

The problem seems to be that we evaluate a vector constant
{-1, 1, -1, 1} loaded with length 11 + 1(bias) = 12 as
{1, -1, 1} instead of {-1, 1, -1}.

Richard wrote on the mailing list:
> The error is probably in vn_reference_lookup_3 which assumes that
> 'len' applies to the vector elements in element order.  See the part
> of the code where it checks for internal_store_fn_p.  If 'len' is with
> respect to the memory and thus endianess has to be taken into
> account then for the IFN_LEN_STORE
> 
>             else if (fn == IFN_LEN_STORE)
>               {
>                 pd.rhs_off = 0;
>                 pd.offset = offset2i;
>                 pd.size = (tree_to_uhwi (len)
>                            + -tree_to_shwi (bias)) * BITS_PER_UNIT;
>                 if (ranges_known_overlap_p (offset, maxsize,
>                                             pd.offset, pd.size))
>                   return data->push_partial_def (pd, set, set,
>                                                  offseti, maxsizei);
> 
> likely needs to adjust rhs_off from zero for big endian?

[Bug middle-end/107617] SCC-VN with len_store and big endian

[rdapp at gcc dot gnu.org]

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

rdapp at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P4

[Bug middle-end/107617] SCC-VN with len_store and big endian

[rdapp at gcc dot gnu.org]

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

--- Comment #1 from rdapp at gcc dot gnu.org ---
For completeness, the mailing list thread is here:

https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602252.html

[Bug tree-optimization/107617] SCC-VN with len_store and big endian

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
                 CC|richard.guenther at gmail dot com  |rguenth at gcc dot gnu.org
   Last reconfirmed|                            |2022-11-11
          Component|middle-end                  |tree-optimization
           Priority|P4                          |P3

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Should be also reproducible on ppc64 big-endian?  Or does that not have
len_store enabled?

Can you try reducing the fortran testcase or create a C testcase that has
the actual miscompilation separated in a function?

[Bug tree-optimization/107617] SCC-VN with len_store and big endian

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
I suppose it's the

+  MEM <vector(4) integer(kind=4)> [(integer(kind=4) *)_13] = { -1, 1, -1, 1 };
...
+  .LEN_STORE (vectp.75_247, 64B, 11, { 255, 255, 255, 255, 0, 0, 0, 1, 255,
255, 255, 255, 0, 0, 0, 1 }, -1);
..
+  MEM <vector(2) integer(kind=8)> [(integer(kind=8) *)&a] = { -1, 1 };
+  MEM <vector(2) integer(kind=8)> [(integer(kind=8) *)&a + 16B] = { -1, 1 };
+  a[4] = 1;
+  a[5] = -1;
+  a[6] = 1;

you are talking about where we elide the scalar loads from _13 stored into
a[].

A gimple testcase would be something like

typedef unsigned char v16qi __attribute__((vector_size(16)));

int a[4];

void __GIMPLE (ssa)
foo (void *p)
{
  int v;

  __BB(2):
  .LEN_STORE (p_1(D), _Literal (int *) 64, 11, _Literal (v16qi) { _Literal
(unsigned char) 255, _Literal (unsigned char) 255, _Literal (unsigned char)
255, _Literal (unsigned char) 255, _Literal (unsigned char) 0, _Literal
(unsigned char) 0, _Literal (unsigned char) 0, _Literal (unsigned char) 1,
_Literal (unsigned char) 255, _Literal (unsigned char) 255, _Literal (unsigned
char) 255, _Literal (unsigned char) 255, _Literal (unsigned char) 0, _Literal
(unsigned char) 0, _Literal (unsigned char) 0, _Literal (unsigned char) 1 },
_Literal (signed char) -1);
  v_2 = __MEM <int> ((int *)p_1(D));
  v_3 = __MEM <int> ((int *)p_1(D) + 4);
  v_4 = __MEM <int> ((int *)p_1(D) + 8);
  v_5 = __MEM <int> ((int *)p_1(D) + 12);
  a[0] = v_2;
  a[1] = v_3;
  a[2] = v_4;
  a[3] = v_5;
  return;
}

which produces

  a[0] = 1;
  a[1] = _Literal (int) -1;
  a[2] = 1;
  a[3] = v_5;

changing the len to 15 and thus folding the .LEN_STORE to a full store changes
that to

  a[0] = _Literal (int) -1;
  a[1] = 1;
  a[2] = _Literal (int) -1;
  a[3] = 1;

which I assume is correct?  I think we'd need to feed a negative pd.rhs_off
into native_encode_expr but that's not supported there (and it treats -1
special).

Still .LEN_STORE covers bytes p + [0..11] here, correct?  But the stored
value is interpreted wrongly here and the new rhs_off assumes little-endian
adjustment.

[Bug tree-optimization/107617] SCC-VN with len_store and big endian

[rguenth at gcc dot gnu.org]

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
Created attachment 53976
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53976&action=edit
patch

Can you please test this patch?  Visually inspecting the fortran testcase and
the GIMPLE testcase shows it fixes this case.

[Bug tree-optimization/107617] SCC-VN with len_store and big endian

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |WAITING

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
Ping - I'm waiting for you to confirm this fixes the issue.

[Bug tree-optimization/107617] SCC-VN with len_store and big endian

[rdapp at linux dot ibm.com]

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

rdapp at linux dot ibm.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rdapp at linux dot ibm.com

--- Comment #6 from rdapp at linux dot ibm.com ---
Thank you and sorry for the delay, I was out on vacation. Will test it soon.

[Bug tree-optimization/107617] SCC-VN with len_store and big endian

[rdapp at linux dot ibm.com]

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

--- Comment #7 from rdapp at linux dot ibm.com ---
The patch fixes the problem for me.  I did a full bootstrap with enabled
len_load support.  No new regressions with -march=arch14.  Thanks again!

[Bug tree-optimization/107617] SCC-VN with len_store and big endian

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 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:d3fee43fb3873b00de913e0501fbf28b56d2ce64

commit r13-4694-gd3fee43fb3873b00de913e0501fbf28b56d2ce64
Author: Richard Biener <rguenther@suse.de>
Date:   Mon Nov 28 12:38:46 2022 +0100

    tree-optimization/107617 - big-endian .LEN_STORE VN

    The following fixes a mistake in interpreting .LEN_STORE definitions
    during value-numbering when in big-endian mode.  We cannot offset
    the encoding of the RHS but instead encode to an offsetted position
    which is then treated correctly by the endian aware copying code.

            PR tree-optimization/107617
            * tree-ssa-sccvn.cc (vn_walk_cb_data::push_partial_def):
            Handle negative pd.rhs_off.
            (vn_reference_lookup_3): Properly provide pd.rhs_off
            for .LEN_STORE on big-endian targets.

[Bug tree-optimization/107617] SCC-VN with len_store and big endian

[rguenth at gcc dot gnu.org]

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

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

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

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