[Bug other/107338] New: new test case gcc.dg/vect/vect-bitfield-read-7.c in r13-3413-ge10ca9544632db fails

[Bug other/107338] New: new test case gcc.dg/vect/vect-bitfield-read-7.c in r13-3413-ge10ca9544632db fails

[seurer at gcc dot gnu.org]

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

            Bug ID: 107338
           Summary: new test case gcc.dg/vect/vect-bitfield-read-7.c in
                    r13-3413-ge10ca9544632db fails
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: other
          Assignee: unassigned at gcc dot gnu.org
          Reporter: seurer at gcc dot gnu.org
  Target Milestone: ---

g:e10ca9544632dbff4759b4b92886cd96d0b9bdfe, r13-3413-ge10ca9544632db
make  -k check-gcc RUNTESTFLAGS="vect.exp=gcc.dg/vect/vect-bitfield-read-7.c"
FAIL: gcc.dg/vect/vect-bitfield-read-7.c execution test
FAIL: gcc.dg/vect/vect-bitfield-read-7.c -flto -ffat-lto-objects execution test
# of expected passes            4
# of unexpected failures        2

I am only seeing this on BE systems (both power 7 and 8) and it fails the same
way for both 32 and 64 bits.  There are no failure messages on the execution.

I tried a debug compilation but the traceback is not helpful:

seurer@makalu-lp1:~/gcc/git/build/gcc-test$
/home/seurer/gcc/git/build/gcc-test/gcc/xgcc
-B/home/seurer/gcc/git/build/gcc-test/gcc/
/home/seurer/gcc/git/gcc-test/gcc/testsuite/gcc.dg/vect/vect-bitfield-read-7.c 
-m64  -fdiagnostics-plain-output  -flto -ffat-lto-objects -maltivec
-mpower8-vector -ftree-vectorize -fno-tree-loop-distribute-patterns
-fno-vect-cost-model -fno-common -O2 -g3 -fdump-tree-vect-details  -lm  -o
./vect-bitfield-read-7.exe

seurer@makalu-lp1:~/gcc/git/build/gcc-test$ gdb ./vect-bitfield-read-7.exe 
(gdb) run
Starting program:
/home/seurer/gcc/git/build/gcc-test/./vect-bitfield-read-7.exe 

Program received signal SIGABRT, Aborted.
0x00003fffb7cd268c in __GI_raise (sig=<optimized out>) at
../nptl/sysdeps/unix/sysv/linux/raise.c:55
55        return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
(gdb) where
#0  0x00003fffb7cd268c in __GI_raise (sig=<optimized out>) at
../nptl/sysdeps/unix/sysv/linux/raise.c:55
#1  0x00003fffb7cd4878 in __GI_abort () at abort.c:90
#2  0x0000000010000600 in .main ()

There's only one abort in the test case on line 37:

  if (f(&A[0], N) != RES)
    abort ();



commit e10ca9544632dbff4759b4b92886cd96d0b9bdfe
Author: Andre Vieira <andre.simoesdiasvieira@arm.com>
Date:   Thu Oct 20 15:54:39 2022 +0100

    vect: Fix vectype when widening container type in bitfield pattern
[PR107326]

[Bug other/107338] new test case gcc.dg/vect/vect-bitfield-read-7.c in r13-3413-ge10ca9544632db fails

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
hmm, there is a plain char there; maybe that is an issue. unsigned vs signed
default on some targets.

[Bug testsuite/107338] new test case gcc.dg/vect/vect-bitfield-read-7.c in r13-3413-ge10ca9544632db fails

[linkw at gcc dot gnu.org]

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

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-10-21
             Status|UNCONFIRMED                 |NEW
                 CC|                            |linkw at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #2 from Kewen Lin <linkw at gcc dot gnu.org> ---
Confirmed, this issue is BE specific.

  vect__ifc__10.14_55 = MEM <vector(8) short unsigned int> [(struct s *)_22];
  vect__ifc__10.15_57 = MEM <vector(8) short unsigned int> [(struct s *)_22 +
16B];
  vect_perm_even_58 = VEC_PERM_EXPR <vect__ifc__10.14_55, vect__ifc__10.15_57,
{ 0, 2, 4, 6, 8, 10, 12, 14 }>;
  vect_patt_7.16_60 = [vec_unpack_hi_expr] vect_perm_even_58;
  vect_patt_7.16_61 = [vec_unpack_lo_expr] vect_perm_even_58;
  vect_patt_30.17_63 = vect_patt_7.16_60 & { 15728640, 15728640, 15728640,
15728640 };
  vect_patt_30.17_64 = vect_patt_7.16_61 & { 15728640, 15728640, 15728640,
15728640 };
  vect_patt_31.18_66 = vect_patt_30.17_63 >> 20;
  vect_patt_31.18_67 = vect_patt_30.17_64 >> 20;

The mask 15728640 (0xF00000) and shifting count 20 look wrong here, the used
prec is incorrect, after the hunk

  /* We move the conversion earlier if the loaded type is smaller than the
     return type to enable the use of widening loads.  */
  if (TYPE_PRECISION (TREE_TYPE (container)) < TYPE_PRECISION (ret_type)
      && !useless_type_conversion_p (TREE_TYPE (container), ret_type))
    {
      pattern_stmt
        = gimple_build_assign (vect_recog_temp_ssa_var (ret_type),
                               NOP_EXPR, container);
      container = gimple_get_lhs (pattern_stmt);
      container_type = TREE_TYPE (container);
      vectype = get_vectype_for_scalar_type (vinfo, container_type);
      append_pattern_def_seq (vinfo, stmt_info, pattern_stmt, vectype);
    }

container_type has become to have precision 32. Although due to widening loads
the resulted vector element have precision 32, actually the low 16 bis are the
data what we want, the prec should use the one before the above adjustment.

If I moved the prec calculation ahead of this hunk, the case can pass.

diff --git a/gcc/tree-vect-patterns.cc b/gcc/tree-vect-patterns.cc
index 777ba2f5903..d7893f7f3bd 100644
--- a/gcc/tree-vect-patterns.cc
+++ b/gcc/tree-vect-patterns.cc
@@ -1925,6 +1925,12 @@ vect_recog_bitfield_ref_pattern (vec_info *vinfo,
stmt_vec_info stmt_info,
   tree container_type = TREE_TYPE (container);
   tree vectype = get_vectype_for_scalar_type (vinfo, container_type);

+  unsigned HOST_WIDE_INT shift_n = bit_field_offset (bf_ref).to_constant ();
+  unsigned HOST_WIDE_INT mask_width = bit_field_size (bf_ref).to_constant ();
+  unsigned HOST_WIDE_INT prec = tree_to_uhwi (TYPE_SIZE (container_type));
+  if (BYTES_BIG_ENDIAN)
+    shift_n = prec - shift_n - mask_width;
+
   /* We move the conversion earlier if the loaded type is smaller than the
      return type to enable the use of widening loads.  */
   if (TYPE_PRECISION (TREE_TYPE (container)) < TYPE_PRECISION (ret_type)
@@ -1953,12 +1959,6 @@ vect_recog_bitfield_ref_pattern (vec_info *vinfo,
stmt_vec_info stmt_info,
        shift_first = false;
     }

-  unsigned HOST_WIDE_INT shift_n = bit_field_offset (bf_ref).to_constant ();
-  unsigned HOST_WIDE_INT mask_width = bit_field_size (bf_ref).to_constant ();
-  unsigned HOST_WIDE_INT prec = tree_to_uhwi (TYPE_SIZE (container_type));
-  if (BYTES_BIG_ENDIAN)
-    shift_n = prec - shift_n - mask_width;
-
   /* If we don't have to shift we only generate the mask, so just fix the
      code-path to shift_first.  */
   if (shift_n == 0)

[Bug testsuite/107338] new test case gcc.dg/vect/vect-bitfield-read-7.c in r13-3413-ge10ca9544632db fails

[avieira at gcc dot gnu.org]

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

--- Comment #3 from avieira at gcc dot gnu.org ---
Hi Kewen,

I believe you are right. I was waiting for a powerpc machine in the board farm,
but I suspect I can reproduce this with an aarch64 BE target and I should be
able to confirm.

But your reasoning seems valid to me. Because of the widening the shift_n
becomes 32-shift_n-mask_width, but the start of the bitfield didn't move by
widening the container, so it is still 16 - shift_n - mask_width bits away from
the start of the container.

Moving the calculation before the widening seems like the neatest solution to
me, there's no point in keeping the old type around I think.

Do you want to produce a patch for this, seeing you solved it?

[Bug testsuite/107338] new test case gcc.dg/vect/vect-bitfield-read-7.c in r13-3413-ge10ca9544632db fails

[linkw at gcc dot gnu.org]

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

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |linkw at gcc dot gnu.org

--- Comment #4 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to avieira from comment #3)
> Hi Kewen,
> 
> I believe you are right. I was waiting for a powerpc machine in the board
> farm, but I suspect I can reproduce this with an aarch64 BE target and I
> should be able to confirm.
> 
> But your reasoning seems valid to me. Because of the widening the shift_n
> becomes 32-shift_n-mask_width, but the start of the bitfield didn't move by
> widening the container, so it is still 16 - shift_n - mask_width bits away
> from the start of the container.
> 
> Moving the calculation before the widening seems like the neatest solution
> to me, there's no point in keeping the old type around I think.
> 
> Do you want to produce a patch for this, seeing you solved it?

Hi Andre,

Thanks for your reply! Sure, I'll make a formal patch and do further
bootstrap/regtest on x86_64-redhat-linux, aarch64-linux-gnu and
powerpc64{,le}-linux-gnu, and post it once the testing go well.

[Bug testsuite/107338] new test case gcc.dg/vect/vect-bitfield-read-7.c in r13-3413-ge10ca9544632db fails

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Kewen Lin <linkw@gcc.gnu.org>:

https://gcc.gnu.org/g:958014f369c8817184af110f8eb2c433a712fd0a

commit r13-3474-g958014f369c8817184af110f8eb2c433a712fd0a
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Tue Oct 25 00:18:08 2022 -0500

    vect: Fix wrong shift_n after widening on BE [PR107338]

    As PR107338 shows, with the use of widening loads, the
    container_type can become a wider type, it causes us to
    get wrong shift_n since the BIT_FIELD_REF offset actually
    becomes bigger on BE.  Taking the case in PR107338 as
    example, at the beginning the container type is short and
    BIT_FIELD_REF offset is 8 and size is 4, with unpacking to
    wider type int, the high 16 bits are zero, by viewing it
    as type int, its offset actually becomes to 24.  So the
    shift_n should be 4 (32 - 24 - 4) instead of 20 (32 - 8
    - 4).

    I noticed that if we move shift_n calculation early
    before the adjustments for widening loads (container type
    change), it's based on all the stuffs of the original
    container, the shfit_n calculated there is exactly what
    we want, it can be independent of widening.  Besides, I
    add prec adjustment together with the current adjustments
    for widening loads, although prec's subsequent uses don't
    require this change for now, since the container type gets
    changed, we should keep the corresponding prec consistent.

            PR tree-optimization/107338

    gcc/ChangeLog:

            * tree-vect-patterns.cc (vect_recog_bitfield_ref_pattern): Move
            shfit_n calculation before the adjustments for widening loads.

[Bug tree-optimization/107338] new test case gcc.dg/vect/vect-bitfield-read-7.c in r13-3413-ge10ca9544632db fails

[linkw at gcc dot gnu.org]

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

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED
          Component|testsuite                   |tree-optimization

--- Comment #6 from Kewen Lin <linkw at gcc dot gnu.org> ---
Should be fixed on trunk now.