Bug 19329 - [3.4 Regression] Bitfield operations cause shifts with 0-count to slip through backends
Summary: [3.4 Regression] Bitfield operations cause shifts with 0-count to slip throug...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 3.4.2
: P3 normal
Target Milestone: 3.4.4
Assignee: Marek Michalkiewicz
URL:
Keywords: missed-optimization
Depends on:
Blocks:
 
Reported: 2005-01-08 17:46 UTC by Giovanni Bajo
Modified: 2005-03-14 09:36 UTC (History)
7 users (show)

See Also:
Host:
Target: avr-elf
Build:
Known to work: 4.0.0
Known to fail: 3.4.2 3.4.3
Last reconfirmed: 2005-01-08 18:52:50


Attachments
Testcase to reproduce the bug (365 bytes, text/plain)
2005-01-08 17:47 UTC, Giovanni Bajo
Details
Generated assembly code (with bug!) (483 bytes, text/plain)
2005-01-08 17:48 UTC, Giovanni Bajo
Details
Patch for 3.4 (also applies to mainline) Superseded by patch for PR19293 (906 bytes, patch)
2005-01-08 20:14 UTC, Bernardo Innocenti
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Giovanni Bajo 2005-01-08 17:46:41 UTC
The testcase I'm going to attach generates a miscompilation on the 3.4 branch. 
The bug cannot be reproduced on 4.0 but might actually be latent there.

The code it's copying 4 1-bit fields from one structure to a different one. The 
internal bit offsets of the fields are different by just one bit between the 
two structures.

This is an except of the generate code:

	lsl r18   # source bitfield disposition is 1bit 
                  # shifited compared to the original, so GCC
                  # adjust it
	andi r24,lo8(-25)   # clear bits it's going to write
	mov r25,r18
	clr r25             # *BUG* it will always write zero
                            # instead of copying the value
	andi r25,lo8(16)
	andi r18,lo8(8)
	or r24,r18          # write correct bit
	or r24,r25          # write zero (sigh!)
Comment 1 Giovanni Bajo 2005-01-08 17:47:36 UTC
Created attachment 7898 [details]
Testcase to reproduce the bug
Comment 2 Giovanni Bajo 2005-01-08 17:48:11 UTC
Created attachment 7899 [details]
Generated assembly code (with bug!)
Comment 3 Giovanni Bajo 2005-01-08 17:50:25 UTC
Compiler details:

Reading specs from /usr/local/avr/lib/gcc/avr/3.4.2/specs
Configured with: ../gcc-3.4.2/configure --host=i686-pc-linux-gnu --
prefix=/usr/local/avr --target=avr --enable-languages=c : (reconfigured) ../gcc-
3.4.2/configure --host=i686-pc-linux-gnu --prefix=/usr/local/avr --target=avr --
enable-languages=c,c++
Thread model: single
gcc version 3.4.2
Comment 4 Bernardo Innocenti 2005-01-08 17:57:13 UTC
Also fails with this pre-release version: 
 
avr-gcc (GCC) 3.4.3 20041019 (prerelease) 
Copyright (C) 2004 Free Software Foundation, Inc. 
This is free software; see the source for copying conditions.  There is NO 
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. 
 
Bootstrapping today's mainline right now. 
 
Comment 5 Andrew Pinski 2005-01-08 18:52:50 UTC
        clr r25  ;  46  ashlqi3/4       [length = 1]

(insn 46 65 49 (set (reg:QI 25 r25 [66])
        (ashift:QI (reg:QI 25 r25 [66])
            (const_int 0 [0x0]))) 57 {ashlqi3} (nil)
    (nil))

So there are two problems here, one a target problem in that it should not be generating a clr for this 
insn.  Two this insn should not be here but that just a misoptimization.
Comment 6 Giovanni Bajo 2005-01-08 19:17:55 UTC
Bernardo's working on a patch.
Comment 7 Bernardo Innocenti 2005-01-08 20:14:16 UTC
Created attachment 7905 [details]
Patch for 3.4 (also applies to mainline)

Superseded by patch for PR19293
Comment 8 Andrew Pinski 2005-01-08 21:19:38 UTC
The shift with zero comes from regmove.
Comment 9 Andrew Pinski 2005-01-09 02:13:54 UTC
Patch posted here: <http://gcc.gnu.org/ml/gcc-patches/2005-01/msg00454.html>.
Comment 10 Björn Haase 2005-01-18 23:35:56 UTC
I have the impression that Bug #19329 is the same as bug #19239 (as one might 
think already when looking at the similarity of the numbers :-) ) 19239, 
howeverr  so far has addressed the issue of *negative* and other really illegal 
shift counts. In a patch treating both problems correctly, one should possibly 
implement both: 
 
Treatment of negative and other illegal shift counts and positive shift counts. 
 
Yours, 
 
Björn. 
 
Comment 11 Björn Haase 2005-01-18 23:43:15 UTC
Sorry for this: 
 
In my posting above, I have misspelled the bug number. I wanted to refer you 
to bug #19293 (and not #19239, luckyly the number of possible permutations is 
countable). 
 
By the way at #19293, you will also find a patch suggestion that should be 
eaysily adapted to all of the present shifting problems. 
 
Yours again, 
 
Björn 
Comment 12 Bernardo Innocenti 2005-01-19 00:03:40 UTC
(In reply to comment #11)

> By the way at #19293, you will also find a patch suggestion that should be 
> eaysily adapted to all of the present shifting problems. 

I agree PR19293 is a superset of this bug, so I'll
close it and withdraw my (already approved) patch.

Could you update your patch with a test for <= 0?
Please post it in gcc-patches, with me and
Marek Michalkiewicz <marekm@amelek.gda.pl> in Cc.

I will take care to install your patch in CVS as
soon as it gets approved.


*** This bug has been marked as a duplicate of 19293 ***
Comment 13 Bernardo Innocenti 2005-01-19 00:06:20 UTC
Oops, I forgot this bug should stay open until someone
figures out why GCC 3.4 leaks through insns with a 0
shift count.

I've reclassified the bug as affecting the middle-end.
Comment 14 Bernardo Innocenti 2005-01-19 00:11:24 UTC
I'm no longer in charge for this bug.
Comment 15 Andrew Pinski 2005-01-19 00:12:56 UTC
(In reply to comment #8)
> The shift with zero comes from regmove.
Well I did figure out where the shift with zero came from see above but why it comes about I don't 
know.
Comment 16 CVS Commits 2005-01-26 21:44:53 UTC
Subject: Bug 19329

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	marekm@gcc.gnu.org	2005-01-26 21:44:26

Modified files:
	gcc            : ChangeLog 
	gcc/config/avr : avr.c avr.md 

Log message:
	PR target/19293
	PR target/19329
	* config/avr/avr.c (notice_update_cc): Only set condition code for
	ashrqi3 if shift count > 0.
	(out_shift_with_cnt): Handle shift count <= 0 as a no-op.
	(ashlqi3_out, ashlhi3_out, ashlsi3_out, ashrqi3_out, ashrhi3_out,
	ashrsi3_out, lshrqi3_out, lshrhi3_out, lshrsi3_out): Handle shift
	count <= 0 as a no-op, and shift count >= width by copying zero
	or sign bit to all bits of the result.
	* config/avr/avr.md (all shifts): Add alternatives for zero shift
	count, with attribute "length" set to 0 and "cc" set to "none".

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.7290&r2=2.7291
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/avr/avr.c.diff?cvsroot=gcc&r1=1.127&r2=1.128
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/avr/avr.md.diff?cvsroot=gcc&r1=1.47&r2=1.48

Comment 17 CVS Commits 2005-01-27 22:36:19 UTC
Subject: Bug 19329

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_4-branch
Changes by:	marekm@gcc.gnu.org	2005-01-27 22:36:00

Modified files:
	gcc            : ChangeLog 
	gcc/config/avr : avr.c avr.md 

Log message:
	PR target/19293
	PR target/19329
	* config/avr/avr.c (notice_update_cc): Only set condition code for
	ashrqi3 if shift count > 0.
	(out_shift_with_cnt): Handle shift count <= 0 as a no-op.
	(ashlqi3_out, ashlhi3_out, ashlsi3_out, ashrqi3_out, ashrhi3_out,
	ashrsi3_out, lshrqi3_out, lshrhi3_out, lshrsi3_out): Handle shift
	count <= 0 as a no-op, and shift count >= width by copying zero
	or sign bit to all bits of the result.
	* config/avr/avr.md (all shifts): Add alternatives for zero shift
	count, with attribute "length" set to 0 and "cc" set to "none".

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=2.2326.2.785&r2=2.2326.2.786
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/avr/avr.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.108.4.3&r2=1.108.4.4
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/avr/avr.md.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.42.4.1&r2=1.42.4.2

Comment 18 Giovanni Bajo 2005-03-09 17:17:39 UTC
Marek, is this bug fixed then? Can we close it?
Comment 19 Marek Michalkiewicz 2005-03-09 17:55:06 UTC
(In reply to comment #18)  
> Marek, is this bug fixed then? Can we close it?  
  
Fixed for the AVR, but the middle-end may still generate these "shift by 0"  
insns - not sure if all other targets handle them correctly. 
 
Comment 20 Giovanni Bajo 2005-03-14 09:36:10 UTC
Fixed then.

Roger, do you believe that middle-end is right in generating these shifts by 
zero? If you believe it is a bug, we can open a new bugreport to track it.