On Sat, Aug 16, 2025 at 7:34 PM Andrew Pinski
<andrew.pin...@oss.qualcomm.com> wrote:
>
> Just like r16-465-gf2bb7ffe84840d8 but this time
> instead of a VCE there is a full on load from a boolean.
> This showed up when trying to remove the extra copy
> in the testcase from the revision mentioned above (pr120122-1.c).
> So when moving loads from a boolean type from being conditional
> to non-conditional, the load needs to become a full load and then
> casted into a bool so that the upper bits are correct.
>
> We could use build_ref_for_offset if that supported a similar ability
> to add the statements into the sequence. Instead we copy a modified
> version that does a similar thing into rewrite_to_defined_unconditional.
>
> Bootstrapped and tested on x86_64-linux-gnu.
>
> PR tree-optimization/121279
>
> gcc/ChangeLog:
>
> * gimple-fold.cc (gimple_needing_rewrite_undefined): Return
> true for boolean loads.
> (rewrite_to_defined_unconditional): Handle boolean loads.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/torture/pr121279-1.c: New test.
>
> Signed-off-by: Andrew Pinski <andrew.pin...@oss.qualcomm.com>
> ---
> gcc/gimple-fold.cc | 113 +++++++++++++++++++++-
> gcc/testsuite/gcc.dg/torture/pr121279-1.c | 49 ++++++++++
> 2 files changed, 161 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.dg/torture/pr121279-1.c
>
> diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> index 85319b33492..40707d03946 100644
> --- a/gcc/gimple-fold.cc
> +++ b/gcc/gimple-fold.cc
> @@ -69,6 +69,7 @@ along with GCC; see the file COPYING3. If not see
> #include "varasm.h"
> #include "internal-fn.h"
> #include "gimple-range.h"
> +#include "alias.h"
>
> enum strlen_range_kind {
> /* Compute the exact constant string length. */
> @@ -10520,6 +10521,19 @@ gimple_needing_rewrite_undefined (gimple *stmt)
> && !POINTER_TYPE_P (lhs_type))
> return false;
> tree rhs = gimple_assign_rhs1 (stmt);
> + /* Boolean loads need special handling as they are treated as a full MODE
> load
> + and don't mask off the bits for the precision. */
> + if (gimple_assign_load_p (stmt)
> + && TREE_CODE (lhs_type) == BOOLEAN_TYPE
I think the relevant thing is the code of the RHS type. Also does this
not apply to non-bitfield loads of !type_has_mode_precision_p types
in general? I am thinking of the fact that we treat a conversion
to/from a same precision/signedness enum/integer type as useless,
so singling out BOOLEAN_TYPE here looks "dangerous" at least?
You are not specifically looking for TYPE_PRECISION == 1 (which is OK
I think).
> + && !type_has_mode_precision_p (lhs_type)
> + /* Loads directly from a decl don't need the
> + rewrite as it should always contain a valid value. */
> + && !DECL_P (rhs)
What's the reasoning here that doesn't apply to other cases
[that are not within a union]? Is it that for a DECL we know
the ref is not inside a union? Wouldn't that extent to all refs
based on a decl? Note at least with C++ you can placement-new
a union { bool b; char c; } onto a 'bool' decl ...
> + /* Bit-fields loads don't need a rewrite as the masking
> + happens for them. */
> + && (TREE_CODE (rhs) != COMPONENT_REF
> + || !DECL_BIT_FIELD (TREE_OPERAND (rhs, 1))))
> + return true;
> /* VCE from integral types to a integral types but with
> a smaller precision need to be changed into casts
> to be well defined. */
> @@ -10558,6 +10572,104 @@ rewrite_to_defined_unconditional
> (gimple_stmt_iterator *gsi, gimple *stmt,
> print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
> }
> gimple_seq stmts = NULL;
> + tree lhs = gimple_assign_lhs (stmt);
> +
> + /* Bool loads need to be rewritten to be a load from the same mode
> + and then a cast to the boolean type so the other bits are masked off
> + correctly since the load was done conditionally. It is similar to the
> VCE
> + case below. */
> + if (gimple_assign_load_p (stmt)
> + && TREE_CODE (TREE_TYPE (lhs)) == BOOLEAN_TYPE)
> + {
> + tree rhs = gimple_assign_rhs1 (stmt);
> + /* Direct reads from decl don't need a rewrite. */
> + gcc_assert (!DECL_P (rhs));
> + /* Bit-fields loads will do the masking so don't need the rewriting.
> */
> + gcc_assert (TREE_CODE (rhs) != COMPONENT_REF
> + || !DECL_BIT_FIELD (TREE_OPERAND (rhs, 1)));
> +
> + location_t loc = gimple_location (stmt);
> + auto bits = GET_MODE_BITSIZE (SCALAR_TYPE_MODE (TREE_TYPE (rhs)));
> + tree new_type = build_nonstandard_integer_type (bits, true);
> + poly_int64 offset;
> + tree base = get_addr_base_and_unit_offset (rhs, &offset);
> + tree off;
> + /* Preserve address-space information. */
> + addr_space_t as = TYPE_ADDR_SPACE (TREE_TYPE (rhs));
> + if (as != TYPE_ADDR_SPACE (new_type))
> + new_type = build_qualified_type (new_type,
> + TYPE_QUALS (new_type)
> + | ENCODE_QUAL_ADDR_SPACE (as));
> +
> + /* get_addr_base_and_unit_offset returns NULL for references with a
> variable
> + offset such as array[var_index]. */
> + if (!base)
> + {
> + tree tmp, addr;
> + gassign *nstmt;
> +
> + /* Create instead `tmp = &array[index];` which then will be
> deferenced. */
> + tmp = make_ssa_name (build_pointer_type (TREE_TYPE (rhs)));
> + addr = build_fold_addr_expr (unshare_expr (rhs));
Ugh, that will make the base addressable. How about, for an outermost
ARRAY_REF, V_C_E its base to an array type with adjusted component?
And for any other handled_component_p with constant offset (ugh,
no good idea for the variable offset component-ref case - building a new
fake FIELD_DECL is a bit too ugly?) replace it with
a bit-field-ref? You need to dive down to the base and replace that with
a MEM_REF with adjusted alias-pointer type.
> + STRIP_USELESS_TYPE_CONVERSION (addr);
> + nstmt = gimple_build_assign (tmp, addr);
> + gimple_set_location (nstmt, loc);
> + if (in_place)
> + gsi_insert_before (gsi, nstmt, GSI_SAME_STMT);
> + else
> + gimple_seq_add_stmt (&stmts, nstmt);
> + off = build_zero_cst (reference_alias_ptr_type (rhs));
> + base = tmp;
> + }
> + /* For [MEM a + 10]->b change it into MEM[a + 10 +offseof(b)] */
> + else if (TREE_CODE (base) == MEM_REF)
> + {
> + off = build_int_cst (TREE_TYPE (TREE_OPERAND (base, 1)),
> + offset);
> + off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1), off);
> + base = unshare_expr (TREE_OPERAND (base, 0));
> + }
> + /* For `a.b` change it into `MEM[&a + offsetof(b)]` */
> + else
> + {
> + off = build_int_cst (reference_alias_ptr_type (rhs),
> + offset);
> + base = build_fold_addr_expr (unshare_expr (base));
> + }
> + /* */
?
> + if (TYPE_ALIGN (new_type) > TYPE_ALIGN (TREE_TYPE (rhs)))
> + new_type = build_aligned_type (new_type, TYPE_ALIGN (TREE_TYPE
> (rhs)));
> + tree mem_ref = fold_build2_loc (loc, MEM_REF, new_type, base, off);
> + if (TREE_THIS_VOLATILE (rhs))
> + TREE_THIS_VOLATILE (mem_ref) = 1;
> + if (TREE_SIDE_EFFECTS (rhs))
> + TREE_SIDE_EFFECTS (mem_ref) = 1;
> +
> + /* Replace the original load with a new load and a new lhs. */
> + tree new_lhs = make_ssa_name (new_type);
> + gimple_assign_set_rhs1 (stmt, mem_ref);
> + gimple_assign_set_lhs (stmt, new_lhs);
> +
> + if (in_place)
> + update_stmt (stmt);
> + else
> + {
> + gimple_set_modified (stmt, true);
> + gimple_seq_add_stmt (&stmts, stmt);
> + }
> +
> + /* Build the conversion statement. */
> + gimple *cvt = gimple_build_assign (lhs, NOP_EXPR, new_lhs);
> + if (in_place)
> + {
> + gsi_insert_after (gsi, cvt, GSI_SAME_STMT);
> + update_stmt (stmt);
> + }
> + else
> + gimple_seq_add_stmt (&stmts, cvt);
> + return stmts;
> + }
> +
> /* VCE from integral types to another integral types but with
> smaller precisions need to be changed into casts
> to be well defined. */
> @@ -10579,7 +10691,6 @@ rewrite_to_defined_unconditional
> (gimple_stmt_iterator *gsi, gimple *stmt,
> }
> return stmts;
> }
> - tree lhs = gimple_assign_lhs (stmt);
> tree type = unsigned_type_for (TREE_TYPE (lhs));
> if (gimple_assign_rhs_code (stmt) == ABS_EXPR)
> gimple_assign_set_rhs_code (stmt, ABSU_EXPR);
> diff --git a/gcc/testsuite/gcc.dg/torture/pr121279-1.c
> b/gcc/testsuite/gcc.dg/torture/pr121279-1.c
> new file mode 100644
> index 00000000000..516b887fe65
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr121279-1.c
> @@ -0,0 +1,49 @@
> +/* { dg-do run } */
> +/* PR tree-optimization/121279 */
> +
> +#include <stdbool.h>
> +
> +struct Value {
> + int type;
> + union {
> + bool boolean;
> + long long t;
> + };
> +};
> +
> +static struct Value s_item_mem;
> +
> +/* truthy was being miscompiled for the value.type==2 case,
> + because the bool load from the union is pulled out of the
> + loop but that was conditional before and now it is not,
> + so turns it into `s_item_mem.type == 1 | bool` which is
> + not valid if `s_item_mem.type == 2` . */
> +static bool truthy(void) __attribute__((noipa));
> +static bool
> +truthy(void)
> +{
> + bool tt = false;
> + for(int i = 0; i < 10; i++)
> + {
> + if (s_item_mem.type == 0)
> + tt = tt | 0;
> + else if (s_item_mem.type == 1)
> + tt = tt | s_item_mem.boolean;
> + else
> + tt = tt | 1;
> + }
> + return tt;
> +}
> +
> +int
> +main(void)
> +{
> + s_item_mem.type = 2;
> + s_item_mem.t = -1;
> + bool b1 = !truthy();
> + s_item_mem.type = 1;
> + s_item_mem.boolean = b1;
> + bool b = truthy();
> + if (b1 != b) __builtin_abort();
> + if (b) __builtin_abort();
> +}
> --
> 2.43.0
>
