public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug target/55147] New: x86: wrong code for 64-bit load
@ 2012-10-31 11:23 mans at mansr dot com
2012-10-31 15:39 ` [Bug target/55147] " jakub at gcc dot gnu.org
` (7 more replies)
0 siblings, 8 replies; 9+ messages in thread
From: mans at mansr dot com @ 2012-10-31 11:23 UTC (permalink / raw)
To: gcc-bugs
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55147
Bug #: 55147
Summary: x86: wrong code for 64-bit load
Classification: Unclassified
Product: gcc
Version: 4.8.0
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: target
AssignedTo: unassigned@gcc.gnu.org
ReportedBy: mans@mansr.com
> cat foo.c
unsigned foo(unsigned long long *p, int i)
{
return __builtin_bswap64(p[i]);
}
> gcc-4.8.0 -m32 -O1 -S foo.c
foo:
movl 8(%esp), %edx
movl 4(%esp), %eax
movl (%eax,%edx,8), %edx
movl 4(%eax,%edx,8), %eax
bswap %eax
ret
Note the first movl overwriting the index register %edx.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Bug target/55147] x86: wrong code for 64-bit load
2012-10-31 11:23 [Bug target/55147] New: x86: wrong code for 64-bit load mans at mansr dot com
@ 2012-10-31 15:39 ` jakub at gcc dot gnu.org
2012-10-31 15:48 ` ubizjak at gmail dot com
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: jakub at gcc dot gnu.org @ 2012-10-31 15:39 UTC (permalink / raw)
To: gcc-bugs
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55147
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|UNCONFIRMED |NEW
Last reconfirmed| |2012-10-31
CC| |jakub at gcc dot gnu.org,
| |uros at gcc dot gnu.org
Ever Confirmed|0 |1
--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-10-31 15:37:49 UTC ---
--- gcc/config/i386/i386.md.jj 2012-10-30 09:01:15.000000000 +0100
+++ gcc/config/i386/i386.md 2012-10-31 16:25:13.986916767 +0100
@@ -12688,7 +12688,14 @@ (define_insn_and_split "*bswapdi2_double
(set (match_dup 0)
(bswap:SI (match_dup 3)))]
{
- split_double_mode (DImode, &operands[0], 2, &operands[0], &operands[2]);
+ split_double_mode (DImode, &operands[0], 1, &operands[0], &operands[2]);
+ if (MEM_P (operands[1])
+ && reg_overlap_mentioned_p (operands[2], operands[1]))
+ {
+ emit_insn (gen_rtx_SET (VOIDmode, operands[0], XEXP (operands[1], 0)));
+ operands[1] = change_address (operands[1], VOIDmode, operands[0]);
+ }
+ split_double_mode (DImode, &operands[1], 1, &operands[1], &operands[3]);
if (REG_P (operands[0]) && REG_P (operands[1]))
{
can fix the reg overlap problem between address of operands[1] and high part of
operands[0]. That said, I wonder what is the advantage of having bswapdi2
patter on i?86 at all that the generic expand_doubleword_bswap can't handle.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Bug target/55147] x86: wrong code for 64-bit load
2012-10-31 11:23 [Bug target/55147] New: x86: wrong code for 64-bit load mans at mansr dot com
2012-10-31 15:39 ` [Bug target/55147] " jakub at gcc dot gnu.org
@ 2012-10-31 15:48 ` ubizjak at gmail dot com
2012-10-31 16:07 ` jakub at gcc dot gnu.org
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: ubizjak at gmail dot com @ 2012-10-31 15:48 UTC (permalink / raw)
To: gcc-bugs
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55147
--- Comment #2 from Uros Bizjak <ubizjak at gmail dot com> 2012-10-31 15:47:46 UTC ---
> can fix the reg overlap problem between address of operands[1] and high part of
> operands[0]. That said, I wonder what is the advantage of having bswapdi2
> patter on i?86 at all that the generic expand_doubleword_bswap can't handle.
If generic code creates similar assembly, then I'm all for removing this
pattern.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Bug target/55147] x86: wrong code for 64-bit load
2012-10-31 11:23 [Bug target/55147] New: x86: wrong code for 64-bit load mans at mansr dot com
2012-10-31 15:39 ` [Bug target/55147] " jakub at gcc dot gnu.org
2012-10-31 15:48 ` ubizjak at gmail dot com
@ 2012-10-31 16:07 ` jakub at gcc dot gnu.org
2012-11-01 9:27 ` [Bug target/55147] [4.8 Regression] " ubizjak at gmail dot com
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: jakub at gcc dot gnu.org @ 2012-10-31 16:07 UTC (permalink / raw)
To: gcc-bugs
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55147
--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-10-31 16:07:11 UTC ---
For the testcase from this PR it creates better assembly actually (compared to
with the #c1 patch, without that it is both longer and wrong). That is because
when bswapdi is split too late, nothing optimizes the fact that only 32 bits of
the result are used.
For
unsigned long long
f1 (unsigned long long *p, int i)
{
return __builtin_bswap64 (p[i]);
}
unsigned long long
f2 (unsigned long long p)
{
return __builtin_bswap64 (p);
}
void
f3 (unsigned long long *p, int i, unsigned long long q)
{
p[i] = __builtin_bswap64 (q);
}
void
f4 (unsigned long long *p, int i, unsigned long long *q)
{
p[i] = __builtin_bswap64 (q[i]);
}
it creates the same number of insns/same quality (just slightly different RA
decisions/scheduling) for f1-f3, but for f4 without bswapdi2 it creates
slightly worse code (with bswapdi2 f4 needs just one call saved register,
without it two, supposedly because both bswap insns are scheduled together.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Bug target/55147] [4.8 Regression] x86: wrong code for 64-bit load
2012-10-31 11:23 [Bug target/55147] New: x86: wrong code for 64-bit load mans at mansr dot com
` (2 preceding siblings ...)
2012-10-31 16:07 ` jakub at gcc dot gnu.org
@ 2012-11-01 9:27 ` ubizjak at gmail dot com
2012-11-01 9:44 ` jakub at gcc dot gnu.org
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: ubizjak at gmail dot com @ 2012-11-01 9:27 UTC (permalink / raw)
To: gcc-bugs
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55147
Uros Bizjak <ubizjak at gmail dot com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|--- |4.8.0
Summary|x86: wrong code for 64-bit |[4.8 Regression] x86: wrong
|load |code for 64-bit load
--- Comment #4 from Uros Bizjak <ubizjak at gmail dot com> 2012-11-01 09:27:21 UTC ---
(In reply to comment #3)
> it creates the same number of insns/same quality (just slightly different RA
> decisions/scheduling) for f1-f3, but for f4 without bswapdi2 it creates
> slightly worse code (with bswapdi2 f4 needs just one call saved register,
> without it two, supposedly because both bswap insns are scheduled together.
We can live with that.
I have also checked that removing the pattern doesn't degrade TARGET_MOVBE, the
reason for their existence is PR53227.
Also, a regression from 4.7.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Bug target/55147] [4.8 Regression] x86: wrong code for 64-bit load
2012-10-31 11:23 [Bug target/55147] New: x86: wrong code for 64-bit load mans at mansr dot com
` (3 preceding siblings ...)
2012-11-01 9:27 ` [Bug target/55147] [4.8 Regression] " ubizjak at gmail dot com
@ 2012-11-01 9:44 ` jakub at gcc dot gnu.org
2012-11-01 9:49 ` ubizjak at gmail dot com
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: jakub at gcc dot gnu.org @ 2012-11-01 9:44 UTC (permalink / raw)
To: gcc-bugs
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55147
--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-01 09:44:22 UTC ---
Created attachment 28589
--> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28589
gcc48-pr55147.patch
So like this? Or do you want to merge the bswap{si,di}2 expanders using SWI48
iterator too? That would make i386.md tiny bit shorter, but would make
gen_bswapdi2 longer (as the compiler can't figure out that for DImode (thus
TARGET_64BIT) TARGET_BSWAP is always true). Perhaps I could do
else if (<MODE>mode == DImode || TARGET_BSWAP)
so that at least optimized gcc builds would optimize it away.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Bug target/55147] [4.8 Regression] x86: wrong code for 64-bit load
2012-10-31 11:23 [Bug target/55147] New: x86: wrong code for 64-bit load mans at mansr dot com
` (4 preceding siblings ...)
2012-11-01 9:44 ` jakub at gcc dot gnu.org
@ 2012-11-01 9:49 ` ubizjak at gmail dot com
2012-11-02 8:03 ` jakub at gcc dot gnu.org
2012-11-07 10:15 ` jakub at gcc dot gnu.org
7 siblings, 0 replies; 9+ messages in thread
From: ubizjak at gmail dot com @ 2012-11-01 9:49 UTC (permalink / raw)
To: gcc-bugs
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55147
--- Comment #6 from Uros Bizjak <ubizjak at gmail dot com> 2012-11-01 09:48:50 UTC ---
(In reply to comment #5)
> Created attachment 28589 [details]
> gcc48-pr55147.patch
>
> So like this? Or do you want to merge the bswap{si,di}2 expanders using SWI48
> iterator too? That would make i386.md tiny bit shorter, but would make
> gen_bswapdi2 longer (as the compiler can't figure out that for DImode (thus
> TARGET_64BIT) TARGET_BSWAP is always true). Perhaps I could do
> else if (<MODE>mode == DImode || TARGET_BSWAP)
> so that at least optimized gcc builds would optimize it away.
No, your proposed patch is OK and pre-approved for mainline SVN.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Bug target/55147] [4.8 Regression] x86: wrong code for 64-bit load
2012-10-31 11:23 [Bug target/55147] New: x86: wrong code for 64-bit load mans at mansr dot com
` (5 preceding siblings ...)
2012-11-01 9:49 ` ubizjak at gmail dot com
@ 2012-11-02 8:03 ` jakub at gcc dot gnu.org
2012-11-07 10:15 ` jakub at gcc dot gnu.org
7 siblings, 0 replies; 9+ messages in thread
From: jakub at gcc dot gnu.org @ 2012-11-02 8:03 UTC (permalink / raw)
To: gcc-bugs
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55147
--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-02 08:03:05 UTC ---
Author: jakub
Date: Fri Nov 2 08:03:02 2012
New Revision: 193090
URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193090
Log:
PR target/55147
* config/i386/i386.md (bswapdi2): Limit to TARGET_64BIT.
(*bswapdi2_doubleword): Removed.
* gcc.target/i386/pr55147.c: New test.
Added:
trunk/gcc/testsuite/gcc.target/i386/pr55147.c
Modified:
trunk/gcc/ChangeLog
trunk/gcc/config/i386/i386.md
trunk/gcc/testsuite/ChangeLog
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Bug target/55147] [4.8 Regression] x86: wrong code for 64-bit load
2012-10-31 11:23 [Bug target/55147] New: x86: wrong code for 64-bit load mans at mansr dot com
` (6 preceding siblings ...)
2012-11-02 8:03 ` jakub at gcc dot gnu.org
@ 2012-11-07 10:15 ` jakub at gcc dot gnu.org
7 siblings, 0 replies; 9+ messages in thread
From: jakub at gcc dot gnu.org @ 2012-11-07 10:15 UTC (permalink / raw)
To: gcc-bugs
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55147
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution| |FIXED
--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-11-07 10:15:41 UTC ---
Fixed.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-11-07 10:15 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-31 11:23 [Bug target/55147] New: x86: wrong code for 64-bit load mans at mansr dot com
2012-10-31 15:39 ` [Bug target/55147] " jakub at gcc dot gnu.org
2012-10-31 15:48 ` ubizjak at gmail dot com
2012-10-31 16:07 ` jakub at gcc dot gnu.org
2012-11-01 9:27 ` [Bug target/55147] [4.8 Regression] " ubizjak at gmail dot com
2012-11-01 9:44 ` jakub at gcc dot gnu.org
2012-11-01 9:49 ` ubizjak at gmail dot com
2012-11-02 8:03 ` jakub at gcc dot gnu.org
2012-11-07 10:15 ` jakub at gcc dot gnu.org
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).