On Tue, 7 Apr 2020, Richard Biener wrote:
> On April 7, 2020 6:25:26 PM GMT+02:00, Martin Jambor <mjam...@suse.cz> wrote:
> >Hi,
> >
> >On Tue, Apr 07 2020, Richard Biener wrote:
> >> On Tue, 7 Apr 2020, Martin Jambor wrote:
> >>
> >>> Hi,
> >>>
> >>> when sra_modify_expr is invoked on an expression that modifies only
> >>> part of the underlying replacement, such as a BIT_FIELD_REF on a LHS
> >>> of an assignment and the SRA replacement's type is not compatible
> >with
> >>> what is being replaced (0th operand of the B_F_R in the above
> >>> example), it does not work properly, basically throwing away the
> >partd
> >>> of the expr that should have stayed intact.
> >>>
> >>> This is fixed in two ways. For BIT_FIELD_REFs, which operate on the
> >>> binary image of the replacement (and so in a way serve as a
> >>> VIEW_CONVERT_EXPR) we just do not bother with convertsing. For
> >>> REALPART_EXPRs and IMAGPART_EXPRs, we insert a VIEW_CONVERT_EXPR
> >under
> >>> the complex partial access expression, which is OK even in a LHS of
> >an
> >>> assignment (and other write contexts) because in those cases the
> >>> replacement is not going to be a giple register anyway.
> >>>
> >>> The testcase for handling REALPART_EXPR and IMAGPART_EXPR is a bit
> >>> fragile because SRA prefers complex and vector types over anything
> >else
> >>> (and in between the two it decides based on TYPE_UID which in my
> >testing
> >>> today always preferred complex types) and even when GIMPLE is wrong
> >I
> >>> often still did not get failing runs, so I only run it at -O1 (which
> >is
> >>> the only level where the the test fails for me).
> >>>
> >>> Bootstrapped and tested on x86_64-linux, bootstrap and testing on
> >>> i686-linux and aarch64-linux underway.
> >>>
> >>> OK for trunk (and subsequently for release branches) if it passes?
> >>
> >> OK.
> >
> >I must have been already half asleep when writing that it passed
> >testing. It did not, testsuite/gcc.c-torture/compile/pr42196-3.c fails
> >even on x86_64, because fwprop happily combines
> >
> > u$ci_10 = MEM[(union U *)&u];
> >
> >and
> >
> > f1_6 = IMAGPART_EXPR <VIEW_CONVERT_EXPR<complex float>(u$ci_10)>;
> >
> >into
> >
> > f1_6 = IMAGPART_EXPR <MEM[(union U *)&u]>;
> >
> >and the gimple verifier of course does not like that. I'll have a look
> >at that tomorrow.
>
> Ah, that might be a genuine fwprop bug.
On a second thought
f1_6 = IMAGPART_EXPR <VIEW_CONVERT_EXPR<complex float>(u$ci_10)>;
shouldn't be valid GIMPLE since 'complex float' is a register type
and thus it should have been
_11 = VIEW_CONVERT_EXPR<complex float>(u$ci_10);
f1_6 = IMAGPART_EXPR <_11>;
now it's a bit of a grey area since a load like
f1_6 = IMAGPART_EXPR <VIEW_CONVERT_EXPR<complex float>(u);
is valid (we don't want to force a whole _Complex load here).
For example in VN
f1_6 = IMAGPART_EXPR <VIEW_CONVERT_EXPR<complex float>(u$ci_10)>;
goes through the load machinery.
So let's say the IL is undesirable at least.
The following fixes the forwprop laziness, please include it in the
patch so it gets cherry-picked for backports.
Thanks,
Richard.
>From 29348ec5efbbc0430714a2929c6b44d174f78533 Mon Sep 17 00:00:00 2001
From: Richard Biener <rguent...@suse.de>
Date: Wed, 8 Apr 2020 10:23:35 +0200
Subject: [PATCH] Properly verify forwprop merging into REAL/IMAGPART and
BIT_FIELD_REF
To: gcc-patches@gcc.gnu.org
2020-04-08 Richard Biener <rguent...@suse.de>
* tree-ssa-forwprop.c (pass_forwprop::execute): Properly
verify the first operand of combinations into REAL/IMAGPART_EXPR
and BIT_FIELD_REF.
---
gcc/tree-ssa-forwprop.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/gcc/tree-ssa-forwprop.c b/gcc/tree-ssa-forwprop.c
index e7eaa18ccad..3d8acf7eb03 100644
--- a/gcc/tree-ssa-forwprop.c
+++ b/gcc/tree-ssa-forwprop.c
@@ -2815,7 +2815,8 @@ pass_forwprop::execute (function *fun)
continue;
if (!is_gimple_assign (use_stmt)
|| (gimple_assign_rhs_code (use_stmt) != REALPART_EXPR
- && gimple_assign_rhs_code (use_stmt) !=
IMAGPART_EXPR))
+ && gimple_assign_rhs_code (use_stmt) != IMAGPART_EXPR)
+ || TREE_OPERAND (gimple_assign_rhs1 (use_stmt), 0) != lhs)
{
rewrite = false;
break;
@@ -2877,7 +2878,8 @@ pass_forwprop::execute (function *fun)
if (is_gimple_debug (use_stmt))
continue;
if (!is_gimple_assign (use_stmt)
- || gimple_assign_rhs_code (use_stmt) != BIT_FIELD_REF)
+ || gimple_assign_rhs_code (use_stmt) != BIT_FIELD_REF
+ || TREE_OPERAND (gimple_assign_rhs1 (use_stmt), 0) != lhs)
{
rewrite = false;
break;
--
2.16.4
