On Tue, May 20, 2025 at 11:29:10AM +0800, Yang Yujie wrote:
> gcc/c-family/ChangeLog:
>
> * c-common.cc (resolve_overloaded_atomic_exchange): Truncate
> _BitInt values before atomic store.
You aren't truncating _BitInt values before atomic store, you are extending
them.
> (resolve_overloaded_atomic_compare_exchange): Same.
>
> gcc/ChangeLog:
>
> * explow.cc (promote_function_mode): Allow _BitInt types
> to be promoted.
> (promote_mode): Same.
> * expr.cc (expand_expr_real_1): Do not truncate _BitInts
> from ABI bondaries if the target sets the "extended" flag.
> (EXTEND_BITINT): Same.
> * gimple-lower-bitint.cc (struct bitint_large_huge):
> Access the highest-order limb of a large/huge _BitInt using limb
> type rather than a new type with reduced precision if _BitInt(N)
> is extended by definition.
8 spaces instead of tabs on the 3 lines above.
Though, as I wrote, I'd really like to see that those changes in
gimple-lower-bitint.cc are really needed. Plus I've committed large
changes to the file today for big endian support, so the patch won't apply
cleanly most likely (and needs to take into account endianity for the
extensions when really needed).
> --- a/gcc/c-family/c-common.cc
> +++ b/gcc/c-family/c-common.cc
> @@ -8033,11 +8033,36 @@ resolve_overloaded_atomic_exchange (location_t loc,
> tree function,
> /* Convert new value to required type, and dereference it.
> If *p1 type can have padding or may involve floating point which
> could e.g. be promoted to wider precision and demoted afterwards,
> - state of padding bits might not be preserved. */
> + state of padding bits might not be preserved.
> +
> + However, as a special case, we still want to preserve the padding
> + bits of _BitInt values if the ABI requires them to be extended in
> + memory. */
> +
> build_indirect_ref (loc, p1, RO_UNARY_STAR);
> - p1 = build2_loc (loc, MEM_REF, I_type,
> - build1 (VIEW_CONVERT_EXPR, I_type_ptr, p1),
> - build_zero_cst (TREE_TYPE (p1)));
> +
> + tree p1type = TREE_TYPE (p1);
> + bool bitint_extended_p = false;
> + if (TREE_CODE (TREE_TYPE (p1type)) == BITINT_TYPE)
> + {
> + struct bitint_info info;
> + unsigned prec = TYPE_PRECISION (TREE_TYPE (p1type));
> + targetm.c.bitint_type_info (prec, &info);
> + bitint_extended_p = info.extended;
> + }
> +
> + if (bitint_extended_p)
The indentation of the above line looks wrong.
> + p1 = build1_loc (loc, CONVERT_EXPR, I_type,
Shouldn't this be fold_convert_loc instead?
> + build2_loc (loc, MEM_REF, TREE_TYPE (p1type),
> + p1, build_zero_cst (p1type)));
> +
Why the empty line in the middle of if/else?
> + /* Otherwise, the padding bits might not be preserved, as stated above. */
> + else
> + p1 = build2_loc (loc, MEM_REF, I_type,
> + build1 (VIEW_CONVERT_EXPR, I_type_ptr, p1),
> + build_zero_cst (p1type));
> +
> +
Just one empty line, not two?
> +
> + if (bitint_extended_p)
Again.
> + p2 = build1_loc (loc, CONVERT_EXPR, I_type,
Again.
> + build2_loc (loc, MEM_REF, TREE_TYPE (p2type),
> + p2, build_zero_cst (p2type)));
> +
Again.
> + /* Otherwise, the padding bits might not be preserved, as stated above. */
> + else
> + p2 = build2_loc (loc, MEM_REF, I_type,
> + build1 (VIEW_CONVERT_EXPR, I_type_ptr, p2),
> + build_zero_cst (p2type));
> +
> (*params)[2] = p2;
>
> /* The rest of the parameters are fine. NULL means no special return value
> --- a/gcc/explow.cc
> +++ b/gcc/explow.cc
> @@ -852,11 +852,26 @@ promote_function_mode (const_tree type, machine_mode
> mode, int *punsignedp,
> return mode;
> }
>
> + /* Handle _BitInt(N) that does not require promotion. */
> + if (TREE_CODE (type) == BITINT_TYPE)
> + {
> + if (TYPE_MODE (type) == BLKmode)
> + return mode;
> +
> + struct bitint_info info;
> + bool ok = targetm.c.bitint_type_info (TYPE_PRECISION (type), &info);
> + gcc_assert (ok);
> +
> + if (!info.extended)
> + return mode;
> + }
There is a switch below, so IMNSHO the BITINT_TYPE handling should go into
the switch.
> +
> +
> switch (TREE_CODE (type))
> {
So perhaps better add it here like:
case BITINT_TYPE:
if (TYPE_MODE (type) == BLKmode)
return mode;
else
{
struct bitint_info info;
bool ok = targetm.c.bitint_type_info (TYPE_PRECISION (type), &info);
gcc_assert (ok);
if (!info.extended)
return mode;
}
/* FALLTHRU */
> case INTEGER_TYPE: case ENUMERAL_TYPE: case BOOLEAN_TYPE:
> case REAL_TYPE: case OFFSET_TYPE: case FIXED_POINT_TYPE:
> - case POINTER_TYPE: case REFERENCE_TYPE:
> + case POINTER_TYPE: case REFERENCE_TYPE: case BITINT_TYPE:
> return targetm.calls.promote_function_mode (type, mode, punsignedp,
> funtype,
> for_return);
>
> @@ -891,10 +906,25 @@ promote_mode (const_tree type ATTRIBUTE_UNUSED,
> machine_mode mode,
> code = TREE_CODE (type);
> unsignedp = *punsignedp;
>
> + /* Handle _BitInt(N) that does not require promotion. */
> + if (code == BITINT_TYPE)
> + {
> + if (TYPE_MODE (type) == BLKmode)
> + return mode;
> +
> + struct bitint_info info;
> + bool ok = targetm.c.bitint_type_info (TYPE_PRECISION (type), &info);
> + gcc_assert (ok);
> +
> + if (!info.extended)
> + return mode;
> + }
> +
> switch (code)
> {
Ditto.
> case INTEGER_TYPE: case ENUMERAL_TYPE: case BOOLEAN_TYPE:
> case REAL_TYPE: case OFFSET_TYPE: case FIXED_POINT_TYPE:
> + case BITINT_TYPE:
> /* Values of these types always have scalar mode. */
> smode = as_a <scalar_mode> (mode);
> PROMOTE_MODE (smode, unsignedp, type);
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -11237,6 +11237,10 @@ expand_expr_real_1 (tree exp, rtx target,
> machine_mode tmode,
> tree ssa_name = NULL_TREE;
> gimple *g;
>
> + type = TREE_TYPE (exp);
> + mode = TYPE_MODE (type);
> + unsignedp = TYPE_UNSIGNED (type);
> +
> /* Some ABIs define padding bits in _BitInt uninitialized. Normally, RTL
> expansion sign/zero extends integral types with less than mode precision
> when reading from bit-fields and after arithmetic operations (see
> @@ -11247,8 +11251,10 @@ expand_expr_real_1 (tree exp, rtx target,
> machine_mode tmode,
> objects in memory, or function arguments, return value). Because we
> internally extend after arithmetic operations, we can avoid doing that
> when reading from SSA_NAMEs of vars. */
> +
> #define EXTEND_BITINT(expr) \
> ((TREE_CODE (type) == BITINT_TYPE \
> + && !bitint_type_info.extended \
> && reduce_bit_field
> \
> && mode != BLKmode
> \
> && modifier != EXPAND_MEMORY \
> @@ -11257,9 +11263,13 @@ expand_expr_real_1 (tree exp, rtx target,
> machine_mode tmode,
> && modifier != EXPAND_CONST_ADDRESS) \
> ? reduce_to_bit_field_precision ((expr), NULL_RTX, type) : (expr))
>
> - type = TREE_TYPE (exp);
> - mode = TYPE_MODE (type);
> - unsignedp = TYPE_UNSIGNED (type);
> + struct bitint_info bitint_type_info;
> + if (TREE_CODE (type) == BITINT_TYPE)
> + {
> + bool ok = targetm.c.bitint_type_info (TYPE_PRECISION (type),
> + &bitint_type_info);
> + gcc_assert (ok);
IMHO we don't want to do this all the time.
Plus I think we shouldn't support targets where the target hook initializes
info->big_endian or info->extended members depending on the n argument, I
think all targets should depending on the ABI set those regardless of the n
argument.
So, I'd cache it in some global tri-state variable and reset that var e.g.
when starting the expand pass for a particular function to unknown state,
and then it can be handled inside of EXTEND_BITINT macro, call there some
inline function to handle it.
> @@ -578,7 +580,7 @@ bitint_large_huge::~bitint_large_huge ()
> void
> bitint_large_huge::insert_before (gimple *g)
> {
> - gimple_set_location (g, m_loc);
> + gimple_set_location (g, m_loc) ;
Please drop this.
> gsi_insert_before (&m_gsi, g, GSI_SAME_STMT);
> }
>
> @@ -587,18 +589,34 @@ bitint_large_huge::insert_before (gimple *g)
> significant limb if any. */
>
> tree
> -bitint_large_huge::limb_access_type (tree type, tree idx)
> +bitint_large_huge::limb_access_type (tree type, tree idx,
> + bool force_extend)
> {
> if (type == NULL_TREE)
> return m_limb_type;
> +
> + /* Either force_extend or m_force_extend should be set
> + when we want this function to return the type of the
> + truncated partial (high) limb. */
> + force_extend |= m_force_extend;
> +
> unsigned HOST_WIDE_INT i = tree_to_uhwi (idx);
> unsigned int prec = TYPE_PRECISION (type);
> gcc_assert (i * limb_prec < prec);
> if ((i + 1) * limb_prec <= prec)
> return m_limb_type;
> else
> - return build_nonstandard_integer_type (prec % limb_prec,
> - TYPE_UNSIGNED (type));
> + {
> + struct bitint_info info;
> + bool ok = targetm.c.bitint_type_info (prec, &info);
> + gcc_assert (ok);
We definitely don't want to call this all the time. Remember
info.extended once in the same place like I now initialize
bitint_big_endian, call it bitint_extended.
> + if (info.extended && !force_extend)
> + return m_limb_type;
As I wrote earlier, this won't apply anyway, it has been changed
for big endian.
So this is primarly meant as an optimization, so that code when
reading an already written upper limb on info.extended target won't
try to extend it again? I'd think an optimization should be deferred
for later, first make sure it sign/zero extends everywhere where it
must according to ABI and then incrementally optimize further
(the expr.cc changes after caching are perhaps ok).
Anyway, I wonder if for the extension cases you always want m_limb_type
returned, why the callers just don't use it.
> + else
> + return build_nonstandard_integer_type (prec % limb_prec,
> + TYPE_UNSIGNED (type));
> + }
> }
>
> /* Return a tree how to access limb IDX of VAR corresponding to BITINT_TYPE
> @@ -1126,6 +1144,42 @@ bitint_large_huge::add_cast (tree type, tree val)
> tree
> bitint_large_huge::handle_plus_minus (tree_code code, tree rhs1, tree rhs2,
> tree idx)
> +{
> + /* Truncate the result if the target ABI requires so. */
> + struct bitint_info info;
> + bool ok = targetm.c.bitint_type_info (
> + TYPE_PRECISION (rhs1 == NULL_TREE
> + ? TREE_TYPE (rhs2) : TREE_TYPE (rhs1)),
Please avoid calls with ( at the end of line if at all possible, this
needs to be cached, and I really don't understand why the +/- code doesn't
already properly extend.
> + &info);
> +
> + gcc_assert (ok);
> +
> + bool m_force_extend_save = m_force_extend;
> + if (info.extended)
> + m_force_extend = true;
> +
> + rhs2 = handle_operand (rhs2, idx);
> + if (rhs1 == NULL_TREE)
> + rhs1 = build_zero_cst (TREE_TYPE (rhs2));
> + else
> + rhs1 = handle_operand (rhs1, idx);
> +
> + if (info.extended)
> + m_force_extend = m_force_extend_save;
> +
> + tree ret = handle_plus_minus_1 (code, rhs1, rhs2, idx);
> +
> + if (info.extended && !m_force_extend
> + && !types_compatible_p (TREE_TYPE (ret), m_limb_type))
> + ret = add_cast (m_limb_type, ret);
> +
> + return ret;
> +}
> +
> +
Functions need function comments.
> +tree
> +bitint_large_huge::handle_plus_minus_1 (tree_code code, tree rhs1, tree rhs2,
> + tree idx)
> {
> tree lhs, data_out, ctype;
> tree rhs1_type = TREE_TYPE (rhs1);
> @@ -1234,9 +1288,27 @@ bitint_large_huge::handle_lshift (tree rhs1, tree
> rhs2, tree idx)
> {
> unsigned HOST_WIDE_INT cnt = tree_to_uhwi (rhs2);
> gcc_checking_assert (cnt < (unsigned) limb_prec);
> +
Why?
Jakub
