[Bug rtl-optimization/65067] regression on accessing volatile bit field

[Bug rtl-optimization/65067] regression on accessing volatile bit field

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65067

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
             Target|                            |arm-eabi

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
This looks more like a failure to use bfi rather than shifts and bit
operations.

[Bug rtl-optimization/65067] regression on accessing volatile bit field

[terry.guo at arm dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65067

--- Comment #2 from Terry Guo <terry.guo at arm dot com> ---
(In reply to Richard Biener from comment #1)
> This looks more like a failure to use bfi rather than shifts and bit
> operations.

If the above IF clause returns false, which means we don't need to consider
strict bit field, the gcc will try to check whether we can use BFI instruction.
Is it a good idea to do same check and attempt when IF clause returns True?

[Bug rtl-optimization/65067] regression on accessing volatile bit field

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65067

--- Comment #3 from rguenther at suse dot de <rguenther at suse dot de> ---
On Mon, 16 Feb 2015, terry.guo at arm dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65067
> 
> --- Comment #2 from Terry Guo <terry.guo at arm dot com> ---
> (In reply to Richard Biener from comment #1)
> > This looks more like a failure to use bfi rather than shifts and bit
> > operations.
> 
> If the above IF clause returns false, which means we don't need to consider
> strict bit field, the gcc will try to check whether we can use BFI instruction.
> Is it a good idea to do same check and attempt when IF clause returns True?

I think actual code generation routines should be (are?) shared.
Definitely bfi should be tried instead of producing the rest.
It seems bfi doesn't work on memory anyway (or is that exposed only
very late?)

[Bug rtl-optimization/65067] regression on accessing volatile bit field

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65067

Bernd Edlinger <bernd.edlinger at hotmail dot de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bernd.edlinger at hotmail dot de

--- Comment #4 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
this does not happen with 4.9.0,
but I can confirm this behavior with current trunk.

[Bug rtl-optimization/65067] regression on accessing volatile bit field

[tony.liu at arm dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65067

Tony Liu <tony.liu at arm dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tony.liu at arm dot com

--- Comment #5 from Tony Liu <tony.liu at arm dot com> ---
(In reply to Bernd Edlinger from comment #4)
> this does not happen with 4.9.0,
> but I can confirm this behavior with current trunk.

Yes. This does not happen before the revision 216989, which is committed after
4.9.2 release. 
And I think Richard's suggestion is the right way to fix this bug. I'll try my
best to implement the patch.

[Bug rtl-optimization/65067] regression on accessing volatile bit field

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65067

--- Comment #6 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
Ok, I think I understand now, what is wrong.

r216989 did just cause the strict-alignment code path to be executed,
which was not the case before.

Actually the extract_bit_field code is also wrong, but the combine pass
replaces the two instruction sequence and/shift with ubfx, but for the
store_bit_field the generated code is way too complex for the combine pass.


So, this would by my first idea, how to fix it:


Index: expmed.c
===================================================================
--- expmed.c    (revision 221087)
+++ expmed.c    (working copy)
@@ -1080,6 +1080,15 @@ store_fixed_bit_field_1 (rtx op0, unsigned HOST_WI
   mode = GET_MODE (op0);
   gcc_assert (SCALAR_INT_MODE_P (mode));

+  if (MEM_P (op0) && bitsize < GET_MODE_BITSIZE (mode))
+    {
+      temp = copy_to_reg (op0);
+      store_bit_field_1 (temp, bitsize, bitnum, 0, 0,
+             mode, value, true);
+      emit_move_insn (op0, temp);
+      return;
+    }
+
   /* Note that bitsize + bitnum can be greater than GET_MODE_BITSIZE (mode)
      for invalid input, such as f5 from gcc.dg/pr48335-2.c.  */

@@ -1852,6 +1861,14 @@ extract_fixed_bit_field_1 (machine_mode tmode, rtx
   machine_mode mode = GET_MODE (op0);
   gcc_assert (SCALAR_INT_MODE_P (mode));

+  if (MEM_P (op0) && bitsize < GET_MODE_BITSIZE (mode))
+    {
+      op0 = copy_to_reg (op0);
+      return extract_bit_field_1 (op0, bitsize, bitnum,
+                  unsignedp, target,
+                  mode, tmode, true);
+    }
+
   /* Note that bitsize + bitnum can be greater than GET_MODE_BITSIZE (mode)
      for invalid input, such as extract equivalent of f5 from
      gcc.dg/pr48335-2.c.  */

[Bug rtl-optimization/65067] regression on accessing volatile bit field

[tony.liu at arm dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65067

--- Comment #7 from Tony Liu <tony.liu at arm dot com> ---
(In reply to Bernd Edlinger from comment #6)
> Ok, I think I understand now, what is wrong.
> 
> r216989 did just cause the strict-alignment code path to be executed,
> which was not the case before.
> 
> Actually the extract_bit_field code is also wrong, but the combine pass
> replaces the two instruction sequence and/shift with ubfx, but for the
> store_bit_field the generated code is way too complex for the combine pass.
> 
> 
> So, this would by my first idea, how to fix it:
> 
> 
> Index: expmed.c
> ===================================================================
> --- expmed.c	(revision 221087)
> +++ expmed.c	(working copy)
> @@ -1080,6 +1080,15 @@ store_fixed_bit_field_1 (rtx op0, unsigned HOST_WI
>    mode = GET_MODE (op0);
>    gcc_assert (SCALAR_INT_MODE_P (mode));
>  
> +  if (MEM_P (op0) && bitsize < GET_MODE_BITSIZE (mode))
> +    {
> +      temp = copy_to_reg (op0);
> +      store_bit_field_1 (temp, bitsize, bitnum, 0, 0,
> +			 mode, value, true);
> +      emit_move_insn (op0, temp);
> +      return;
> +    }
> +
>    /* Note that bitsize + bitnum can be greater than GET_MODE_BITSIZE (mode)
>       for invalid input, such as f5 from gcc.dg/pr48335-2.c.  */
>  
> @@ -1852,6 +1861,14 @@ extract_fixed_bit_field_1 (machine_mode tmode, rtx
>    machine_mode mode = GET_MODE (op0);
>    gcc_assert (SCALAR_INT_MODE_P (mode));
>  
> +  if (MEM_P (op0) && bitsize < GET_MODE_BITSIZE (mode))
> +    {
> +      op0 = copy_to_reg (op0);
> +      return extract_bit_field_1 (op0, bitsize, bitnum,
> +				  unsignedp, target,
> +				  mode, tmode, true);
> +    }
> +
>    /* Note that bitsize + bitnum can be greater than GET_MODE_BITSIZE (mode)
>       for invalid input, such as extract equivalent of f5 from
>       gcc.dg/pr48335-2.c.  */

I've checked your patch on trunk, it works and also passes the regression tests
for target Cortex-M3. It will be very appreciated if you could commit this
patch to trunk. Thank you very much.
>From gcc-bugs-return-479272-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Wed Mar 04 05:22:08 2015
Return-Path: <gcc-bugs-return-479272-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 118608 invoked by alias); 4 Mar 2015 05:22:08 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 118495 invoked by uid 48); 4 Mar 2015 05:22:04 -0000
From: "law at redhat dot com" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug middle-end/65233] [5 Regression] ICE (segfault) on arm-linux-gnueabihf and aarch64-linux-gnu
Date: Wed, 04 Mar 2015 05:22:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: middle-end
X-Bugzilla-Version: 5.0
X-Bugzilla-Keywords: ice-on-valid-code
X-Bugzilla-Severity: normal
X-Bugzilla-Who: law at redhat dot com
X-Bugzilla-Status: RESOLVED
X-Bugzilla-Priority: P1
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: 5.0
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-65233-4-jmrEkEVMq6@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-65233-4@http.gcc.gnu.org/bugzilla/>
References: <bug-65233-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2015-03/txt/msg00416.txt.bz2
Content-length: 3138

https://gcc.gnu.org/bugzilla/show_bug.cgi?ide233

--- Comment #21 from Jeffrey A. Law <law at redhat dot com> ---
I was a bit concerned that something here might be more deeply amiss, so I took
a little time to walk through things under the debugger.


If I put a breakpoint in cleanup_tree_cfg and ignore it until the call that
faults...  Then a bit of debugging yields:

(gdb) p debug_bb_n (30)
<bb 30>:
# SR.33_23 = PHI <>
# SR.32_80 = PHI <>
goto <bb 12>;

(gdb) p debug_bb_n (13)
<bb 13>:
# iftmp.4_25 = PHI <iftmp.4_20(42), iftmp.4_22(45), iftmp.4_98(46)>
# SR.33_83 = PHI <SR.33_23(42), SR.33_52(45), SR.33_52(46)>
# SR.32_16 = PHI <SR.32_80(42), SR.32_81(45), SR.32_81(46)>
_18 = SR.33_83->_vptr.C;
_19 = *_18;
OBJ_TYPE_REF(_19;(struct C)SR.33_83->0) (SR.33_83);
goto <bb 15>;

So, that's consistent with what I expected.  The key being BB30 defines _23,
but is unreachable...

After removing the unreachable blocks and computing dominators we have:

<bb 13>:
# iftmp.4_25 = PHI <iftmp.4_20(42), iftmp.4_22(45), iftmp.4_98(46)>
# SR.33_83 = PHI <SR.33_23(42), SR.33_52(45), SR.33_52(46)>
# SR.32_16 = PHI <SR.32_80(42), SR.32_81(45), SR.32_81(46)>
_18 = SR.33_83->_vptr.C;
_19 = *_18;
OBJ_TYPE_REF(_19;(struct C)SR.33_83->0) (SR.33_83);
goto <bb 15>;

So there's still a reference to SR.33_23, but BB13 (the defining block for
SR.33_23) has been removed.

When we removed BB13, we tried to release SR.33_23, but it's queued for SSA
updating:

(gdb) p ((*cfun)->gimple_df->ssa_names).m_vecdata[23]
$46 = (tree_node *) 0x7ffff65e1ea0
(gdb) p name_registered_for_update_p ($46)
$47 = true

And that's the key here.  We are at the point where the SSA graph is
potentially in consistent (SSA update occurs after CFG cleanup).  This is the
situation I was hinting at in IRC today Aldy.  The fact that SSA updates occur
after cfgcleanup implies that the SSA graph can have some inconsistencies
(such as an SSA_NAME with no defining statement).  Anything called during
cfgcleanup has to be prepared to handle these inconsistencies (and yes, it's
incredibly lame to have these kind of inconsistencies).

So diving deeper, we're trying to merge bb47 and bb13:

(gdb) p debug_bb (a)
<bb 47>:
# iftmp.4_55 = PHI <-1(48)>
goto <bb 13>;

$70 = void
(gdb) p debug_bb (b)
<bb 13>:
# SR.33_83 = PHI <SR.33_23(47)>
# SR.32_16 = PHI <SR.32_80(47)>
_18 = SR.33_83->_vptr.C;
_19 = *_18;
OBJ_TYPE_REF(_19;(struct C)SR.33_23->0) (SR.33_23);
goto <bb 15>;


Which is very reasonable.  In the case where the successor has PHIs we try to
propagate the PHI RHS.  Ie, we'll try to replace _83 with _23 in the call.
That in turn triggers the polymorphic call code that walks the up the use-def
chain via walk_ssa_copies.  walk_ssa_copies has to be prepared to find an
SSA_NAME (_23 in this case) in an inconsistent state (such as being defined in
an unreachable block by a PHI with no RHS arguments).

I think Aldy's patch is fine for this issue.  However, I certainly worry that
with the design issue that has the SSA graph in an inconsistent state that
we'll likely run into other problems with code that's not prepared to handle
the inconsistencies.

[Bug rtl-optimization/65067] regression on accessing volatile bit field

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65067

--- Comment #8 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
Created attachment 34955
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34955&action=edit
Proposed Fix

Well, I noticed that the first version of this patch caused
a small but measurable decrease of code quality on x86_64,
so I moved the patch to the if (strict_volatile_bitfield_p block,
and used some code transformations to simplify the resulting code
a bit.

I will post this new version for review, after a full boot-strap
and successful regression-test on my ARM target.

[Bug rtl-optimization/65067] regression on accessing volatile bit field

[tony.liu at arm dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65067

--- Comment #9 from Tony Liu <tony.liu at arm dot com> ---
(In reply to Bernd Edlinger from comment #8)
> Created attachment 34955 [details]
> Proposed Fix
> 
> Well, I noticed that the first version of this patch caused
> a small but measurable decrease of code quality on x86_64,
> so I moved the patch to the if (strict_volatile_bitfield_p block,
> and used some code transformations to simplify the resulting code
> a bit.
> 
> I will post this new version for review, after a full boot-strap
> and successful regression-test on my ARM target.

I've tested for target Cortex-M3, no regression.

[Bug rtl-optimization/65067] regression on accessing volatile bit field

[edlinger at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65067

--- Comment #11 from Bernd Edlinger <edlinger at gcc dot gnu.org> ---
Author: edlinger
Date: Thu Mar  5 18:56:37 2015
New Revision: 221222

URL: https://gcc.gnu.org/viewcvs?rev=221222&root=gcc&view=rev
Log:
gcc:
2015-03-05  Bernd Edlinger  <bernd.edlinger@hotmail.de>

        PR rtl-optimization/65067
        * expmed.c (store_bit_field, extract_bit_field): Reworked the
        strict volatile bitfield handling.

testsuite:
2015-03-05  Bernd Edlinger  <bernd.edlinger@hotmail.de>

        * gcc.target/arm/pr65067.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/arm/pr65067.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/expmed.c
    trunk/gcc/testsuite/ChangeLog

[Bug rtl-optimization/65067] regression on accessing volatile bit field

[ramana at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65067

Ramana Radhakrishnan <ramana at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
                 CC|                            |ramana at gcc dot gnu.org
         Resolution|---                         |FIXED
   Target Milestone|---                         |5.0

--- Comment #12 from Ramana Radhakrishnan <ramana at gcc dot gnu.org> ---
fixed ?

[Bug rtl-optimization/65067] regression on accessing volatile bit field

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65067

--- Comment #13 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
(In reply to Ramana Radhakrishnan from comment #12)
> fixed ?

yes, thanks.