[Bug target/15184] [4.3/4.4/4.5/4.6/4.7 Regression] Direct access to byte inside word not working with -march=pentiumpro

[Bug target/15184] [4.3/4.4/4.5/4.6/4.7 Regression] Direct access to byte inside word not working with -march=pentiumpro

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15184

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.3.6                       |4.4.7

--- Comment #18 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-06-27 12:13:11 UTC ---
4.3 branch is being closed, moving to 4.4.7 target.

[Bug target/15184] [4.5/4.6/4.7/4.8 Regression] Direct access to byte inside word not working with -march=pentiumpro

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15184

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.4.7                       |4.5.4

--- Comment #19 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-03-13 12:45:59 UTC ---
4.4 branch is being closed, moving to 4.5.4 target.

[Bug target/15184] [4.6/4.7/4.8 Regression] Direct access to byte inside word not working with -march=pentiumpro

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15184

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.5.4                       |4.6.4

[Bug target/15184] [4.6/4.7/4.8 Regression] Direct access to byte inside word not working with -march=pentiumpro

[steven at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15184

Steven Bosscher <steven at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P5                          |P3
   Last reconfirmed|2005-05-28 18:17:13         |2012-11-09 18:17:13
                 CC|                            |steven at gcc dot gnu.org
      Known to fail|                            |

--- Comment #20 from Steven Bosscher <steven at gcc dot gnu.org> 2012-11-09 22:32:30 UTC ---
(In reply to comment #13)

This is indeed pretty bad. Works with GCC 4.1, fails with GCC 4.3 and
with current top-of-trunk, and for all i686 variants including relatively
new CPUs based on the P6 micro-architecture, like core2 and atom.

I disagree with comment #10, and I don't think this problem is minor.  The
code of the test case is common idiom that should be optimized properly.
Please can the RMs have a new look at this.

[Bug target/15184] [4.6/4.7/4.8 Regression] Direct access to byte inside word not working with -march=pentiumpro

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15184

--- Comment #21 from Uros Bizjak <ubizjak at gmail dot com> 2012-11-10 12:15:14 UTC ---
(In reply to comment #20)

> Please can the RMs have a new look at this.

This is tuning decision, and I see Intel folks in the CC. I see no problem in
changing these defaults, if they are supported by some benchmark results.

[Bug target/15184] [4.6/4.7/4.8 Regression] Direct access to byte inside word not working with -march=pentiumpro

[mikpe at it dot uu.se]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15184

--- Comment #22 from Mikael Pettersson <mikpe at it dot uu.se> 2012-11-10 13:36:46 UTC ---
Created attachment 28655
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28655
another test case

I'm using a construct similar to the 'f1' function of the initial test case to
set the low 8 or 16 bits of a 32-bit "register" in a CPU emulator of mine, and
the code generated by gcc 4.6/4.7/4.8 for this on x86_64 is appalling.

In the attached test case, the setb1 and setw1 functions use bit and/or
operations, while the setb2 and setw2 functions assign the sub-field directly
via a union.  gcc compiles each set*2 function to a single mov (+ ret), while
each set*1 function  becomes 5 instructions (+ ret).

[Bug target/15184] [4.6/4.7/4.8 Regression] Direct access to byte inside word not working with -march=pentiumpro

[steven at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15184

Steven Bosscher <steven at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|steven at gcc dot gnu.org   |

--- Comment #23 from Steven Bosscher <steven at gcc dot gnu.org> 2012-11-11 21:24:36 UTC ---
(In reply to comment #21)
> (In reply to comment #20)
> 
> > Please can the RMs have a new look at this.
> 
> This is tuning decision, and I see Intel folks in the CC. I see no problem
> in changing these defaults, if they are supported by some benchmark results.

The original report was probably a tuning issue, but the code from
comment #13 (which is similar to what trunk produces) looks like more
than a tuning issue to me.

[Bug target/15184] [4.6/4.7/4.8 Regression] Direct access to byte inside word not working with -march=pentiumpro

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15184

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug target/15184] [4.6/4.7/4.8 Regression] Direct access to byte inside word not working with -march=pentiumpro

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15184

--- Comment #24 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-15 14:31:02 UTC ---
We fail to fold

  (short unsigned int) ( X | (signed short) Y )

to

  (short unsigned int ) X | (short unsigned int) Y

and thus end up with needlessly many conversions on the tree level.

Index: gcc/convert.c
===================================================================
--- gcc/convert.c       (revision 195194)
+++ gcc/convert.c       (working copy)
@@ -750,7 +750,7 @@ convert_to_integer (tree type, tree expr
                                || ex_form == MULT_EXPR)))
                      typex = unsigned_type_for (typex);
                    else
-                     typex = signed_type_for (typex);
+                     ;
                    return convert (type,
                                    fold_build2 (ex_form, typex,
                                                 convert (typex, arg0),

"fixes" that (but has no effect on the resulting assembly).

Btw, even generic tuning assembler is bad which is IMHO the important fact.

I don't think combine is the right mechanism to deal with this as
partial register writes are certainly not wanted.  Instead it's necessary
to see that we can store into memory directly which requires us to combine
too much things.

This is rather to be dealt with on the tree level - the bswap pass
infrastructure should provide a starting point for value composition
like this.

[Bug target/15184] [4.7/4.8/4.9 Regression] Direct access to byte inside word not working with -march=pentiumpro

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15184

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.6.4                       |4.7.4

--- Comment #25 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-04-12 15:16:13 UTC ---
GCC 4.6.4 has been released and the branch has been closed.

[Bug target/15184] [4.7/4.8/4.9/4.10 Regression] Direct access to byte inside word not working with -march=pentiumpro

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.7.4                       |4.8.4

--- Comment #26 from Richard Biener <rguenth at gcc dot gnu.org> ---
The 4.7 branch is being closed, moving target milestone to 4.8.4.

[Bug target/15184] [4.8/4.9/5 Regression] Direct access to byte inside word not working with -march=pentiumpro

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.4                       |4.8.5

--- Comment #27 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 4.8.4 has been released.

[Bug target/15184] [4.8/4.9/5 Regression] Direct access to byte inside word not working with -march=pentiumpro

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |thomas.preudhomme at arm dot com

--- Comment #28 from Richard Biener <rguenth at gcc dot gnu.org> ---
c.f. comment#24 and using bswap for this kind of value composition

gu (unsigned char c)
{
  short unsigned int y.7_2;
  signed short y.8_3;
  signed short _4;
  int _6;
  int _7;
  signed short _8;
  signed short _9;
  short unsigned int y.9_10;

  <bb 2>:
  y.7_2 = y;
  y.8_3 = (signed short) y.7_2;
  _4 = y.8_3 & 255;
  _6 = (int) c_5(D);
  _7 = _6 << 8;
  _8 = (signed short) _7;
  _9 = _4 | _8;
  y.9_10 = (short unsigned int) _9;
  y = y.9_10;
  return;

this could be

  BIT_FIELD_REF <y, ....> = c_5;

or if not both stores and loads are seen at least

  y.7_2 = y;
  y.9_10 = BIT_FIELD_EXPR <y.7_2, ..., c_5(D)>;
  y = y.9_10;

with the new BIT_FIELD_EXPR code I proposed some time ago for bitfield
insertion.

Technically not limited to byte offsets of course, so maybe not a 100%
fit for the bswap pass.  And of course the BIT_FIELD_EXPR would still
need to be expanded properly (extv/insv support would fit naturally).

Without BIT_FIELD_EXPR the bswap pass could see if the final value
has a single use in a store and then still use a BIT_FIELD_REF on that.

[Bug target/15184] [4.8/4.9/5 Regression] Direct access to byte inside word not working with -march=pentiumpro

[law at redhat dot com]

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

Jeffrey A. Law <law at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |law at redhat dot com

--- Comment #29 from Jeffrey A. Law <law at redhat dot com> ---
I can certainly understand wanting to handle this in the bswap pass and if you
can make it fly there, that's obviously good.

But I also wonder if we're just being dumb in combine and the x86 backend in
terms of recognizing when a blob of RTL really just represents a bitfield
insertion.

Isn't the first case just:

(define_insn_and_split ""
  [(set (match_operand:SI 0 "register_operand")
        (ior:SI (and:SI (match_operand:SI 1 "register_operand")
                        (match_operand 3 "const_int_operand"))
                (zero_extend:SI (match_operand 2 "register_operand"))))]
  "INTVAL (operands[3]) == ~GET_MODE_MASK (GET_MODE (operands[2]))"
  "#"
  ""
  [(set (match_dup 0) (match_dup 1))
   (set (zero_extract (match_dup 0)
                      (match_dup 4)
                      (const_int 0))
        (match_dup 2))]
"{
  operands[4] = GEN_INT (GET_MODE_BITSIZE (GET_MODE (operands[2])));
}")



The second case is similar, except we have another AND expression rather than
the zero_extend in the pattern because we're inserting into a middle byte.

Cases #3 and #4 appear to be more complex, but that may be a failing in the
combine simplifications and canonicalization.  I haven't looked deeply yet.

[Bug target/15184] [4.8/4.9/5 Regression] Direct access to byte inside word not working with -march=pentiumpro

[law at redhat dot com]

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

Jeffrey A. Law <law at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |law at redhat dot com

--- Comment #30 from Jeffrey A. Law <law at redhat dot com> ---
Actually better to handle this in combine rather than the backend.  The problem
is the "to-be-inserted" bits have different canonical forms depending on where
we're inserting them.  if it's the low bits it might be (subreg....) or a
zero_extend.  If it's middle bits, there's an (and ..) and if its in the high
bits, then there's an (ashift ...)

It gets even uglier when we start looking at the 2nd pair of tests

By nailing it down in combine, other targets benefit as well.  For the
combiner, the first two tests (and a couple of my own) are pretty easy.

It's far enough along that taking ownership seems to make sense.  Attacking the
problem in bswap would be good, but obviously further out.

[Bug target/15184] [4.8/4.9/5 Regression] Direct access to byte inside word not working with -march=pentiumpro

[thopre01 at gcc dot gnu.org]

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

thopre01 at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |thopre01 at gcc dot gnu.org

--- Comment #31 from thopre01 at gcc dot gnu.org ---
(In reply to Jeffrey A. Law from comment #30)
> 
> It's far enough along that taking ownership seems to make sense.  Attacking
> the problem in bswap would be good, but obviously further out.

Feel free to take ownership of this bug. I'll definitely think about this use
case when rewriting the bswap pass into a forward analysis but I'm not sure
when I'll go round to work on this.

Best regards.

[Bug target/15184] [4.8/4.9/5 Regression] Direct access to byte inside word not working with -march=pentiumpro

[law at gcc dot gnu.org]

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

--- Comment #32 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Author: law
Date: Thu Jan 29 14:30:45 2015
New Revision: 220249

URL: https://gcc.gnu.org/viewcvs?rev=220249&root=gcc&view=rev
Log:
    PR target/15184
    * combine.c (try_combine): If I0 is a memory load and I3 a store
    to a related address, increase the "goodness" of doing a 4-insn
    combination with I0-I3.
    (make_field_assignment): Handle SUBREGs in the ior+and case.

    PR target/15184
    * gcc.target/i386/pr15184-1.c: New test.
    * gcc.target/i386/pr15184-2.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/i386/pr15184-1.c
    trunk/gcc/testsuite/gcc.target/i386/pr15184-2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/combine.c
    trunk/gcc/testsuite/ChangeLog

[Bug target/15184] [4.8/4.9 Regression] Direct access to byte inside word not working with -march=pentiumpro

[law at redhat dot com]

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

Jeffrey A. Law <law at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|4.8.5                       |5.0
            Summary|[4.8/4.9/5 Regression]      |[4.8/4.9 Regression] Direct
                   |Direct access to byte       |access to byte inside word
                   |inside word not working     |not working with
                   |with -march=pentiumpro      |-march=pentiumpro

--- Comment #33 from Jeffrey A. Law <law at redhat dot com> ---
Fixed on trunk.  Not planning to backport to release branches (requires 4 insn
combine code).

[Bug target/15184] [4.8/4.9 Regression] Direct access to byte inside word not working with -march=pentiumpro

[uros at gcc dot gnu.org]

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

--- Comment #34 from uros at gcc dot gnu.org ---
Author: uros
Date: Fri Jan 30 10:53:53 2015
New Revision: 220277

URL: https://gcc.gnu.org/viewcvs?rev=220277&root=gcc&view=rev
Log:
    PR target/15184
    * gcc.target/i386/pr15184-1.c: Compile for ia32 target only.
    (dg-options): Remove -m32.
    (dg-final): Scan for "movb %al" only.
    * gcc.target/i386/pr15184-2.c: Ditto.


Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/i386/pr15184-1.c
    trunk/gcc/testsuite/gcc.target/i386/pr15184-2.c