Fix wrong SRA with VIEW_CONVERT_EXPR and reverse SSO
Commit Message
Hi,
most cases of VIEW_CONVERT_EXPRs involving reverse scalar storage order are
disqualified for SRA because they are storage_order_barrier_p, but you can
still have a VIEW_CONVERT_EXPR to a regular composite type being applied to
a component of a record type with reverse scalar storage order, although this
is pretty rare even in Ada (the only idiomatic way I know of is to use 'Valid
on a floating-point component).
In this case, the bypass for !useless_type_conversion_p in sra_modify_assign,
albeit already heavily guarded, triggers and may generate wrong code, e.g. on
the attached testcase, so the patch makes sure that it does only when the SSO
is the same on both side.
Bootstrapped/regtested on x86-64/Linux, OK for the mainline?
2022-05-13 Eric Botcazou <ebotcazou@adacore.com>
* tree-sra.c (sra_modify_assign): Check that the scalar storage order
is the same on the LHS and RHS before rewriting one with the model of
the other.
2022-05-13 Eric Botcazou <ebotcazou@adacore.com>
* gnat.dg/sso17.adb: New test.
Comments
On Fri, May 13, 2022 at 9:57 AM Eric Botcazou via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>
> most cases of VIEW_CONVERT_EXPRs involving reverse scalar storage order are
> disqualified for SRA because they are storage_order_barrier_p, but you can
> still have a VIEW_CONVERT_EXPR to a regular composite type being applied to
> a component of a record type with reverse scalar storage order, although this
> is pretty rare even in Ada (the only idiomatic way I know of is to use 'Valid
> on a floating-point component).
>
> In this case, the bypass for !useless_type_conversion_p in sra_modify_assign,
> albeit already heavily guarded, triggers and may generate wrong code, e.g. on
> the attached testcase, so the patch makes sure that it does only when the SSO
> is the same on both side.
>
> Bootstrapped/regtested on x86-64/Linux, OK for the mainline?
OK. Possibly also qualifies for the branch(es) as wrong-code fix.
Thanks,
Richard.
>
> 2022-05-13 Eric Botcazou <ebotcazou@adacore.com>
>
> * tree-sra.c (sra_modify_assign): Check that the scalar storage order
> is the same on the LHS and RHS before rewriting one with the model of
> the other.
>
>
> 2022-05-13 Eric Botcazou <ebotcazou@adacore.com>
>
> * gnat.dg/sso17.adb: New test.
>
> --
> Eric Botcazou
> OK. Possibly also qualifies for the branch(es) as wrong-code fix.
Thanks. It's not a regression, but I can indeed put in on recent branches.
@@ -4270,32 +4270,31 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator *gsi)
sra_stats.exprs++;
}
- if (modify_this_stmt)
- {
- if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (rhs)))
+ if (modify_this_stmt
+ && !useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (rhs)))
+ {
+ /* If we can avoid creating a VIEW_CONVERT_EXPR, then do so.
+ ??? This should move to fold_stmt which we simply should
+ call after building a VIEW_CONVERT_EXPR here. */
+ if (AGGREGATE_TYPE_P (TREE_TYPE (lhs))
+ && TYPE_REVERSE_STORAGE_ORDER (TREE_TYPE (lhs)) == racc->reverse
+ && !contains_bitfld_component_ref_p (lhs))
{
- /* If we can avoid creating a VIEW_CONVERT_EXPR do so.
- ??? This should move to fold_stmt which we simply should
- call after building a VIEW_CONVERT_EXPR here. */
- if (AGGREGATE_TYPE_P (TREE_TYPE (lhs))
- && !contains_bitfld_component_ref_p (lhs))
- {
- lhs = build_ref_for_model (loc, lhs, 0, racc, gsi, false);
- gimple_assign_set_lhs (stmt, lhs);
- }
- else if (lacc
- && AGGREGATE_TYPE_P (TREE_TYPE (rhs))
- && !contains_vce_or_bfcref_p (rhs))
- rhs = build_ref_for_model (loc, rhs, 0, lacc, gsi, false);
+ lhs = build_ref_for_model (loc, lhs, 0, racc, gsi, false);
+ gimple_assign_set_lhs (stmt, lhs);
+ }
+ else if (lacc
+ && AGGREGATE_TYPE_P (TREE_TYPE (rhs))
+ && TYPE_REVERSE_STORAGE_ORDER (TREE_TYPE (rhs)) == lacc->reverse
+ && !contains_vce_or_bfcref_p (rhs))
+ rhs = build_ref_for_model (loc, rhs, 0, lacc, gsi, false);
- if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (rhs)))
- {
- rhs = fold_build1_loc (loc, VIEW_CONVERT_EXPR, TREE_TYPE (lhs),
- rhs);
- if (is_gimple_reg_type (TREE_TYPE (lhs))
- && TREE_CODE (lhs) != SSA_NAME)
- force_gimple_rhs = true;
- }
+ if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (rhs)))
+ {
+ rhs = fold_build1_loc (loc, VIEW_CONVERT_EXPR, TREE_TYPE (lhs), rhs);
+ if (is_gimple_reg_type (TREE_TYPE (lhs))
+ && TREE_CODE (lhs) != SSA_NAME)
+ force_gimple_rhs = true;
}
}