On Tue, Aug 19, 2025 at 12:26 AM Richard Biener <richard.guent...@gmail.com>
wrote:
> 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).
>
I think it does apply to non-bitfields loads of !type_has_mode_precision_p
in general and not just boolean types.
I will remove the check for BOOLEAN_TYPE.
>
> > + && !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 ...
>
It was to work around the issue where lim would create a temporary variable
which is not renamed yet and we would get an ICE. But I don't think it is
needed due to I am going to change the rewriting to just use a VCE (see
below).
>
> > + /* 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.
>
So it turns out I can just remove this whole thing and just add a VCE
around the original reference and it will work correctly. I didn't think of
that before.
>
> > + 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));
> > + }
> > + /* */
>
> ?
>
I was going to add a comment here but I forgot. It is no longer needed due
to the VCE part.
I should have a new patch submitted this evening for this.
Thanks,
Andrew Pinski
>
> > + 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
> >
>
