[Bug other/108623] New: We need to grow the precision field in tree_type_common for PowerPC

[Bug other/108623] New: We need to grow the precision field in tree_type_common for PowerPC

[meissner at gcc dot gnu.org]

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

            Bug ID: 108623
           Summary: We need to grow the precision field in
                    tree_type_common for PowerPC
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: other
          Assignee: unassigned at gcc dot gnu.org
          Reporter: meissner at gcc dot gnu.org
  Target Milestone: ---

The current patches that have been submitted to the PowerPC back end need to
grow the precision field in the tree_type_common structure (in tree-core.h). 
The current precision field is 10 bits but the type size for the dense math
register is 1,024 bits.

With the current 10 bit value, the precision of the TDO mode becomes 0 instead
of 1,024.  I noticed the tree-ssa-ccp.cc pass was dying with test programs that
use the dense math registers when I run the test on a PowerPC, but they run
fine on the x86_64.

Ultimately I tracked this down to the sext_hwi (sign extend) function in
hwint.h.

The code in hwint.h is:

static inline HOST_WIDE_INT
sext_hwi (HOST_WIDE_INT src, unsigned int prec)
{
  if (prec == HOST_BITS_PER_WIDE_INT)
    return src;
  else
#if defined (__GNUC__)
    {
      /* Take the faster path if the implementation-defined bits it's relying
         on are implemented the way we expect them to be.  Namely, conversion
         from unsigned to signed preserves bit pattern, and right shift of
         a signed value propagates the sign bit.
         We have to convert from signed to unsigned and back, because when left
         shifting signed values, any overflow is undefined behavior.  */
      gcc_checking_assert (prec < HOST_BITS_PER_WIDE_INT);
      int shift = HOST_BITS_PER_WIDE_INT - prec;
      return ((HOST_WIDE_INT) ((unsigned HOST_WIDE_INT) src << shift)) >>
shift;
    }
#else
    {
      /* Fall back to the slower, well defined path otherwise.  */
      gcc_checking_assert (prec < HOST_BITS_PER_WIDE_INT);
      HOST_WIDE_INT sign_mask = HOST_WIDE_INT_1 << (prec - 1);
      HOST_WIDE_INT value_mask = (HOST_WIDE_INT_1U << prec) - HOST_WIDE_INT_1U;
      return (((src & value_mask) ^ sign_mask) - sign_mask);
    }
#endif
}

If the 'prec' argument is 0, the 'shift' variable will become 64.

In C/C++, a 64-bit value that is shifted either left or right by 64 bits is
undefined.  It turns out that the x86_64 will happen to return the original
value if you shift it left 64-bits and then shift it right with sign extension.
 From within the CCP pass, the original value is -1.  On the other hand, the
PowerPC always returns 0.  Since it isn't returning -1, it leads the CCP pass
to create a 0 constant for TDOmode.  But since TDOmode is opaque, converting it
to 0 generates an error when checking is enabled.

The solution is to grow the precision to 11 bits and reduce the
contains_placeholder_bits field to 1 bit.

Alternatively, we could grow the precision to 16 bits, which will cause all
trees to grow slightly, and keep contains_placeholder_bits at 2 bits. I see
references to contains placeholder bits in tree.cc and cp/module.cc, but I
don't see any place that actually sets the field.

[Bug middle-end/108623] We need to grow the precision field in tree_type_common for PowerPC

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|other                       |middle-end
             Target|                            |powerpc*

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
There are 15 spare bits at the end of the bits, a better solution is to
re-order things so precision remains aligned to 16 bits, for example by moving
the
6 bits adjacent to it to the "spare" and extending precision to a full 16 bits.

Like with the following which aligns all >1 bit fields to at least 8 bits

diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index acd8deea34e..e5513208511 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -1686,18 +1686,8 @@ struct GTY(()) tree_type_common {
   tree attributes;
   unsigned int uid;

-  unsigned int precision : 10;
-  unsigned no_force_blk_flag : 1;
-  unsigned needs_constructing_flag : 1;
-  unsigned transparent_aggr_flag : 1;
-  unsigned restrict_flag : 1;
-  unsigned contains_placeholder_bits : 2;
-
+  unsigned int precision : 16;
   ENUM_BITFIELD(machine_mode) mode : 8;
-
-  /* TYPE_STRING_FLAG for INTEGER_TYPE and ARRAY_TYPE.
-     TYPE_CXX_ODR_P for RECORD_TYPE and UNION_TYPE.  */
-  unsigned string_flag : 1;
   unsigned lang_flag_0 : 1;
   unsigned lang_flag_1 : 1;
   unsigned lang_flag_2 : 1;
@@ -1713,12 +1703,22 @@ struct GTY(()) tree_type_common {
      so we need to store the value 32 (not 31, as we need the zero
      as well), hence six bits.  */
   unsigned align : 6;
+  /* TYPE_STRING_FLAG for INTEGER_TYPE and ARRAY_TYPE.
+     TYPE_CXX_ODR_P for RECORD_TYPE and UNION_TYPE.  */
+  unsigned string_flag : 1;
+  unsigned no_force_blk_flag : 1;
+
   unsigned warn_if_not_align : 6;
+  unsigned needs_constructing_flag : 1;
+  unsigned transparent_aggr_flag : 1;
+
+  unsigned contains_placeholder_bits : 2;
+  unsigned restrict_flag : 1;
   unsigned typeless_storage : 1;
   unsigned empty_flag : 1;
   unsigned indivisible_p : 1;
   unsigned no_named_args_stdarg_p : 1;
-  unsigned spare : 15;
+  unsigned spare : 9;

   alias_set_type alias_set;
   tree pointer_to;

[Bug middle-end/108623] We need to grow the precision field in tree_type_common for PowerPC

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org,
                   |                            |rsandifo at gcc dot gnu.org

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Note there's code that might need adjustments.  We for example have

/* Return the number of elements in the VECTOR_TYPE given by NODE.  */

inline poly_uint64
TYPE_VECTOR_SUBPARTS (const_tree node)
{
  STATIC_ASSERT (NUM_POLY_INT_COEFFS <= 2); 
  unsigned int precision = VECTOR_TYPE_CHECK (node)->type_common.precision;
  if (NUM_POLY_INT_COEFFS == 2)
    {
      /* See the corresponding code in SET_TYPE_VECTOR_SUBPARTS for a
         description of the encoding.  */
      poly_uint64 res = 0;
      res.coeffs[0] = HOST_WIDE_INT_1U << (precision & 0xff);
      if (precision & 0x100)
        res.coeffs[1] = HOST_WIDE_INT_1U << (precision & 0xff);
      return res;
    }

which looks odd anyways.  Richard might know where the 10 bits have been
baked in to (ISTR something about the INTEGER_CST encoding stuff here
with the three kinds of "precisions")

[Bug middle-end/108623] We need to grow the precision field in tree_type_common for PowerPC

[rsandifo at gcc dot gnu.org]

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

--- Comment #3 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
The explanation is in the SET_TYPE_VECTOR_SUBPARTS code:

      /* We have two coefficients that are each in the range 1 << [0, 63],
         so supporting all combinations would require 6 bits per coefficient
         and 12 bits in total.  Since the precision field is only 10 bits
         in size, we need to be more restrictive than that.

         At present, coeff[1] is always either 0 (meaning that the number
         of units is constant) or equal to coeff[0] (meaning that the number
         of units is N + X * N for some target-dependent zero-based runtime
         parameter X).  We can therefore encode coeff[1] in a single bit.

         The most compact encoding would be to use mask 0x3f for coeff[0]
         and 0x40 for coeff[1], leaving 0x380 unused.  It's possible to
         get slightly more efficient code on some hosts if we instead
         treat the shift amount as an independent byte, so here we use
         0xff for coeff[0] and 0x100 for coeff[1].  */

If we're happy to extend to 16 bits then things become simpler.
We can just use one byte per coefficient.

[Bug middle-end/108623] We need to grow the precision field in tree_type_common for PowerPC

[meissner at gcc dot gnu.org]

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

--- Comment #4 from Michael Meissner <meissner at gcc dot gnu.org> ---
I must have missed the spare bits.  I think it is better to use the full 16
bits for precision.  I also think your other changes to realign bit fields
greater than 1 bit.

[Bug middle-end/108623] We need to grow the precision field in tree_type_common for PowerPC

[segher at gcc dot gnu.org]

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

--- Comment #5 from Segher Boessenkool <segher at gcc dot gnu.org> ---
The failure was not detected, only things down the road broke up, can we add
something for that?  Just a strategically placed assert should do fine.

Less important if we grow the field all the way to 16 bits, but :-)

[Bug middle-end/108623] We need to grow the precision field in tree_type_common for PowerPC

[meissner at gcc dot gnu.org]

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

Michael Meissner <meissner at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-02-01
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

--- Comment #6 from Michael Meissner <meissner at gcc dot gnu.org> ---
Yes I agree we want an assetion in sext_hwi as well.

Richard, are you going to submit the patch, or did you want me to do it (along
with the assertion)?

[Bug middle-end/108623] We need to grow the precision field in tree_type_common for PowerPC

[meissner at gcc dot gnu.org]

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

--- Comment #7 from Michael Meissner <meissner at gcc dot gnu.org> ---
Created attachment 54387
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54387&action=edit
Proposed patch combining Richard's patch and an assertion.

[Bug middle-end/108623] We need to grow the precision field in tree_type_common for PowerPC

[joseph at codesourcery dot com]

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

--- Comment #8 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
See also bug 102989 (C2x _BitInt) regarding another case for which growing 
TYPE_PRECISION would be useful.

[Bug middle-end/108623] We need to grow the precision field in tree_type_common for PowerPC

[rguenther at suse dot de]

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

--- Comment #9 from rguenther at suse dot de <rguenther at suse dot de> ---
On Wed, 1 Feb 2023, meissner at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108623
> 
> Michael Meissner <meissner at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>    Last reconfirmed|                            |2023-02-01
>      Ever confirmed|0                           |1
>              Status|UNCONFIRMED                 |NEW
> 
> --- Comment #6 from Michael Meissner <meissner at gcc dot gnu.org> ---
> Yes I agree we want an assetion in sext_hwi as well.
> 
> Richard, are you going to submit the patch, or did you want me to do it (along
> with the assertion)?

Please you do it, as far as I understand Richard S. no further adjustment
is necessary but we could simplify some code after the change(?)

[Bug middle-end/108623] We need to grow the precision field in tree_type_common for PowerPC

[rsandifo at gcc dot gnu.org]

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

--- Comment #10 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(In reply to rguenther@suse.de from comment #9)
> Please you do it, as far as I understand Richard S. no further adjustment
> is necessary but we could simplify some code after the change(?)
Yeah.  AFAIK, nothing outside these two macros relies on the representation.

[Bug middle-end/108623] We need to grow the precision field in tree_type_common for PowerPC

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 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:e2b993db57f90fedd1bd7756f7ad4c5bfded4b8f

commit r14-577-ge2b993db57f90fedd1bd7756f7ad4c5bfded4b8f
Author: Michael Meissner <meissner@linux.ibm.com>
Date:   Wed Feb 1 12:30:19 2023 -0500

    Bump up precision size to 16 bits.

    The new __dmr type that is being added as a possible future PowerPC
instruction
    set bumps into a structure field size issue.  The size of the __dmr type is
1024 bits.
    The precision field in tree_type_common is currently 10 bits, so if you
store
    1,024 into field, you get a 0 back.  When you get 0 in the precision field,
the
    ccp pass passes this 0 to sext_hwi in hwint.h.  That function in turn
generates
    a shift that is equal to the host wide int bit size, which is undefined as
    machine dependent for shifting in C/C++.

          int shift = HOST_BITS_PER_WIDE_INT - prec;
          return ((HOST_WIDE_INT) ((unsigned HOST_WIDE_INT) src << shift)) >>
shift;

    It turns out the x86_64 where I first did my tests returns the original
input
    before the two shifts, while the PowerPC always returns 0.  In the ccp
pass, the
    original input is -1, and so it worked.  When I did the runs on the
PowerPC, the
    result was 0, which ultimately led to the failure.

    2023-02-01  Richard Biener  <rguenther@suse.de>
                Michael Meissner  <meissner@linux.ibm.com>

            PR middle-end/108623
            * tree-core.h (tree_type_common): Bump up precision field to 16
bits.
            Align bit fields > 1 bit to at least an 8-bit boundary.

[Bug middle-end/108623] We need to grow the precision field in tree_type_common for PowerPC

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED
   Target Milestone|---                         |14.0

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed for GCC 14