Bug 36359 - missed optimization in some cases with PRE VRP and other passes combined together
Summary: missed optimization in some cases with PRE VRP and other passes combined toge...
Status: RESOLVED DUPLICATE of bug 38789
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.4.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-28 22:12 UTC by Mirco Tischler
Modified: 2009-01-26 10:22 UTC (History)
9 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2008-05-31 15:45:30


Attachments
kernel config-file to reproduce error (12.49 KB, application/octet-stream)
2008-05-28 22:14 UTC, Mirco Tischler
Details
preprocessed sources of urb.c (26.84 KB, text/plain)
2008-05-31 13:58 UTC, Mirco Tischler
Details
Mirco's usbcore.o (71.71 KB, application/octet-stream)
2008-06-10 10:11 UTC, Adrian Bunk
Details
drivers/scsi/sd.c (140.17 KB, text/plain)
2008-11-24 18:33 UTC, Török Edwin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mirco Tischler 2008-05-28 22:12:27 UTC
Hi
I've tested gcc-4.4 with the current developer kernel 2.6.26-rc4 and got the following compile error:

...
Setup is 12236 bytes (padded to 12288 bytes).
System is 2521 kB
CRC 735d1682
Kernel: arch/x86/boot/bzImage is ready  (#62)
  Building modules, stage 2.
  MODPOST 222 modules
ERROR: "____ilog2_NaN" [drivers/usb/core/usbcore.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

This can be avoided when switching on the OPTIMIZE_SIZE kconfig option which in fact replaces the standard -O2 option with -Os, so I think there must be something wrong in what -O2 does. 
This happens only with gcc-4.4 not with gcc-4.3 and before (I've also tested gcc-4.1). The latest gcc-4.4 version I tested was 20080526.

I compiled gcc without any options to ./configure.

I'll try to attach my config file so you might be able to reproduce the error.

This has been reported to lkml. The discussion can be read here: http://lkml.org/lkml/2008/5/25/169.

Thanks
Mirco
Comment 1 Mirco Tischler 2008-05-28 22:14:16 UTC
Created attachment 15695 [details]
kernel config-file to reproduce error
Comment 2 Richard Biener 2008-05-28 22:20:33 UTC
This sounds like a kernel bug.  At least we need preprocessed source of the
source file in drivers/usb/core/usbcore.ko that generates the reference
to ____ilog2_NaN.
Comment 3 Andrew Pinski 2008-05-28 22:52:01 UTC
http://lkml.org/lkml/2008/5/25/233

It says something about __builtin_constant_p being broken but I doubt it really.
Comment 4 Mirco Tischler 2008-05-28 23:08:47 UTC
(In reply to comment #2)
> This sounds like a kernel bug.  At least we need preprocessed source of the
> source file in drivers/usb/core/usbcore.ko that generates the reference
> to ____ilog2_NaN.
> 
Can you advise me please how to create them? I've tried adding -save-temps to the EXTRA_CFLAGS in driver/usb/core/Makefile but that didn't seem to have changed anything. There are only sources and .o/.ko files in that directory.
Comment 5 Mirco Tischler 2008-05-31 13:58:27 UTC
Created attachment 15707 [details]
preprocessed sources of urb.c
Comment 6 Mirco Tischler 2008-05-31 13:59:42 UTC
I was able to find the function in which the error occurs. It's usb_submit_urb in drivers/usb/core/urb.c I think.
The command:
$ gcc -c -save-temps drivers/usb/core/urb.c -I include
executed in the linux-kernel main directory gives me the attached urb.i. Is that what you need? Executing this gives me the following errors, though:
In file included from /home/mirco/source/kernel/linux.git/include/asm/bitops.h:421,
                 from /home/mirco/source/kernel/linux.git/include/linux/bitops.h:17,
                 from /home/mirco/source/kernel/linux.git/include/linux/bitmap.h:7,
                 from /home/mirco/source/kernel/linux.git/include/linux/cpumask.h:90,
                 from /home/mirco/source/kernel/linux.git/include/asm/processor.h:25,
                 from /home/mirco/source/kernel/linux.git/include/linux/prefetch.h:14,
                 from /home/mirco/source/kernel/linux.git/include/linux/list.h:6,
                 from /home/mirco/source/kernel/linux.git/include/linux/module.h:9,
                 from urb.c:1:
/home/mirco/source/kernel/linux.git/include/asm-generic/bitops/fls64.h:33:2: error: #error BITS_PER_LONG not 32 or 64
In file included from /home/mirco/source/kernel/linux.git/include/linux/prefetch.h:14,
                 from /home/mirco/source/kernel/linux.git/include/linux/list.h:6,
                 from /home/mirco/source/kernel/linux.git/include/linux/module.h:9,
                 from urb.c:1:
/home/mirco/source/kernel/linux.git/include/asm/processor.h:152:1: warning: "cache_line_size" redefined
In file included from /home/mirco/source/kernel/linux.git/include/asm/pda.h:7,
                 from /home/mirco/source/kernel/linux.git/include/asm/current_64.h:7,
                 from /home/mirco/source/kernel/linux.git/include/asm/current.h:4,
                 from /home/mirco/source/kernel/linux.git/include/asm/processor.h:15,
                 from /home/mirco/source/kernel/linux.git/include/linux/prefetch.h:14,
                 from /home/mirco/source/kernel/linux.git/include/linux/list.h:6,
                 from /home/mirco/source/kernel/linux.git/include/linux/module.h:9,
                 from urb.c:1:
/home/mirco/source/kernel/linux.git/include/linux/cache.h:64:1: warning: this is the location of the previous definition
In file included from /home/mirco/source/kernel/linux.git/include/linux/module.h:21,
                 from urb.c:1:
/home/mirco/source/kernel/linux.git/include/asm/module.h:70:2: error: #error unknown processor family
Comment 7 Richard Biener 2008-05-31 15:45:30 UTC
Looking at http://www.readcode.org/code/linux-2.6.20/include/linux/log2.h it
seems that either the argument to ilog _is_ zero or the compiler thinks so.
Comment 8 Mirco Tischler 2008-06-08 00:17:33 UTC
I've been able to identify the code where ilog2 is called. If I do not choose usbcore as a module I get the following error:
drivers/built-in.o: In function `usb_submit_urb':
(.text+0xc247f): undefined reference to `____ilog2_NaN'
make: *** [.tmp_vmlinux1] Fehler 1

Observing that function usb_submit_urb() in drivers/usb/core/urb.c shows the interesting lines should be:
	case USB_ENDPOINT_XFER_INT:
		/* too small? */
		if (urb->interval <= 0)
			return -EINVAL;
		/* too big? */
		switch (dev->speed) {
		case USB_SPEED_HIGH:	/* units are microframes */
			/* NOTE usb handles 2^15 */
			if (urb->interval > (1024 * 8))
				urb->interval = 1024 * 8;
			max = 1024 * 8;
			break;
		case USB_SPEED_FULL:	/* units are frames/msec */
		case USB_SPEED_LOW:
			if (xfertype == USB_ENDPOINT_XFER_INT) {
				if (urb->interval > 255)
					return -EINVAL;
				/* NOTE ohci only handles up to 32 */
				max = 128;
			} else {
				if (urb->interval > 1024)
					urb->interval = 1024;
				/* NOTE usb and ohci handle up to 2^15 */
				max = 1024;
			}
			break;
		default:
			return -EINVAL;
		}
		/* Round down to a power of 2, no more than max */
		urb->interval = min(max, 1 << ilog2(urb->interval));
	}

As you can see, the argument to ilog2 is urb->interval which can't be zero at that point IMHO.
Comment 9 pinskia@gmail.com 2008-06-08 00:33:48 UTC
Subject: Re:  [Regression] compile error in linux-kernel 2.6.26-rc4 with -O2

On Sat, Jun 7, 2008 at 5:17 PM, mt-ml at gmx dot de
<gcc-bugzilla@gcc.gnu.org> wrote:
> As you can see, the argument to ilog2 is urb->interval which can't be zero at
> that point IMHO.

Yes and that requires a lot of jump threading and many other
optimizations to detect that really.  First PRE has to be working and
then VRP has to figure out it will be greater than 0.  So really this
code depends on optimization to work correctly and the kernel is
broken that way again (not suppressing really)

-- Pinski
Comment 10 Adrian Bunk 2008-06-10 10:11:58 UTC
Created attachment 15745 [details]
Mirco's usbcore.o
Comment 11 Adrian Bunk 2008-06-10 10:28:50 UTC
(In reply to comment #7)
> Looking at http://www.readcode.org/code/linux-2.6.20/include/linux/log2.h it
> seems that either the argument to ilog _is_ zero or the compiler thinks so.

If you disassemble Mirco's usbcore.o you'll see why I said "I'd have said your gcc has a bug regarding __builtin_constant_p()": The compiler does not think the argument to ilog() was zero - it expanded _all_ the code for the case when __builtin_constant_p(n) returns true.
Comment 12 Bernhard Reutner-Fischer 2008-06-10 11:07:40 UTC
smallish testcase, fwiw.

$ gcc -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -Werror-implicit-function-declaration -fno-stack-protector -mregparm=3 -freg-struct-return -mpreferred-stack-boundary=2  -march=i686 -ffreestanding -pipe -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -O2 -c -o pr.o pr36359.c
$ nm pr.o 
         U argh
00000000 T foo
         U urb
$ cat pr36359.c 
extern int urb;
int argh(void);
void foo(void)
{
	if (urb <= 0)
		return;
	urb = __builtin_constant_p(urb) ? 0x0815 : argh();
}
Comment 13 Bernhard Reutner-Fischer 2008-06-10 11:20:53 UTC
should have been
int argh(void) __attribute__((error ("dce?")));

works without __builtin_constant_p and fails even with
-Os -ftree-dce -fweb -ftree-fre -ftree-pre

pr36359.c: In function 'foo':
pr36359.c:7: error: call to 'argh' declared with attribute error: dce?

Comment 14 Andrew Pinski 2008-11-15 00:28:43 UTC
(In reply to comment #12)
> smallish testcase, fwiw.
No that should have a call to argh.
Comment 15 Andrew Pinski 2008-11-15 00:31:08 UTC
As I mentioned this requires so many optimizations to get this done correctly it is not a regression.
Comment 16 Andrew Pinski 2008-11-15 00:34:47 UTC
The preprocessed source does not compile for me:
/home/mirco/source/kernel/linux.git/include/asm/current_64.h:11: error: expected expression before 'struct'

Comment 17 Török Edwin 2008-11-24 18:33:32 UTC
Created attachment 16761 [details]
drivers/scsi/sd.c

Similar problem with drivers/scsi/sd.c

I can compile the attached preprocessed on x86_64 linux:
$ gcc-4.3 -c x.i -o x.o -O2 && nm -u x.o|grep ilog2
$ ~/gcc_inst/bin/gcc -c x.i -o x.o -O2 && nm -u x.o|grep ilog2
U ____ilog2_NaN

IMHO the kernel is abusing gcc as a static analyzer, which gcc is not.
Comment 18 macius bat 2008-12-17 20:08:04 UTC
confirmed for linux kernel 2.6.28-rc8.
Comment 19 Michal Svoboda 2009-01-25 19:25:04 UTC
This thing still exists in 2.6.28.2; what do you suggest should be changed in the kernel to let it compile again?
Comment 20 Richard Biener 2009-01-25 21:10:08 UTC
The testcase from #17 does not reproduce the issue for me with recent GCC 4.3.
Comment 21 Adrian Bunk 2009-01-25 22:00:09 UTC
(In reply to comment #20)
> The testcase from #17 does not reproduce the issue for me with recent GCC 4.3.

This bug is a regression in gcc 4.4, it was AFAIK never present in gcc 4.3.

Haven't tried more recent gcc versions, but I'm still seeing it with the 20090107-1 gcc-snapshot package (that is the SVN trunk).
Comment 22 Adrian Bunk 2009-01-25 22:05:41 UTC
Check my comments #10 and #11 and the definition of ilog2() in http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=include/linux/log2.h;h=25b808631cd92c50d10cf6a31b2d9b9942b62ac9;hb=bce7f793daec3e65ec5c5705d2457b81fe7b5725 and you'll understand why I said 8 months ago "gcc has a bug regarding __builtin_constant_p()".
Comment 23 Richard Biener 2009-01-25 22:17:55 UTC
It doesn't reproduce for me with 4.4 either.  Maybe this is a dup of PR38789?
Comment 24 Adrian Bunk 2009-01-26 00:49:07 UTC
(In reply to comment #23)
> It doesn't reproduce for me with 4.4 either.  Maybe this is a dup of PR38789?


Seems so:
I've confirmed that the 4.4-20090109 snapshot is broken, and the 4.4-20090123 snapshot is fixed.
Comment 25 Richard Biener 2009-01-26 10:22:08 UTC

*** This bug has been marked as a duplicate of 38789 ***