From: Ulrich Drepper <drepper@redhat.com>
To: Norbert Kiesel <nkiesel@tbdnetworks.com>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Multiple & vs. && and | vs. || bugs in 2.4.20
Date: Sun, 02 Mar 2003 10:05:43 -0800 [thread overview]
Message-ID: <3E6247F7.8060301@redhat.com> (raw)
In-Reply-To: <20030302121425.GA27040@defiant>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Norbert Kiesel wrote:
> --- linux-2.4.20/drivers/usb/acm.c~ 2002-12-03 00:17:50.000000000 -0800
> +++ linux-2.4.20/drivers/usb/acm.c 2003-03-02 03:03:34.000000000 -0800
> @@ -240,7 +240,7 @@
> if (urb->status)
> dbg("nonzero read bulk status received: %d", urb->status);
>
> - if (!urb->status & !acm->throttle) {
> + if (!urb->status && !acm->throttle) {
> for (i = 0; i < urb->actual_length && !acm->throttle; i++) {
> /* if we insert more than TTY_FLIPBUF_SIZE characters,
> * we drop them. */
Have you really looked at detail at all these cases? The problem is
that you have made the code less efficient on some platforms. The use
of && requires shortcut evaluation. I.e., the compiler is forced to not
evaluate !acm->throttle if !urb->status is true. The causes, unless the
architecture has condition bits, a conditional jump.
The original code didn't need and normally has no jump and thi specific
case was certainly fine before since the result of the ! operator is
either 0 or 1 and therefore the & operator has no strange side effects.
As an example, here is how the code for x86 could have looked before
movl status(%edx), %edx
testl %edx, %edx
movl throttle(%eax), %eax
sete %dl
testl %eax, %eax
sete %al
andb %dl, %al
jne ...
[if code]
After the change the code must look something like this:
movl status(%edx), %edx
testl %edx, %edx
jne ...
movl throttle(%eax), %eax
testl %eax, %eax
jne ...
[if code]
Observe the extra 'jne' and the fact that the value of 'throttle'
element cannot be loaded until after the conditional jump. Not even
out of order execution can arrange that.
To summarize, I'd probably not be amused if you would change any of my
code which takes advantage of such programming finess. I would probably
have added appropriate comments to explain the code but nevertheless,
replacing the more efficient code with some which is easier to
understand should probably be considered on a case by case basis.
Incorrect branch prediction is costly.
- --
- --------------. ,-. 444 Castro Street
Ulrich Drepper \ ,-----------------' \ Mountain View, CA 94041 USA
Red Hat `--' drepper at redhat.com `---------------------------
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)
iD8DBQE+Ykf82ijCOnn/RHQRApJaAKCxM4hwu12mJVbQuD3o+t13YVxrsACgsnVH
RZmgjNB5KP3Qu27iqpf5aiU=
=l7xl
-----END PGP SIGNATURE-----
next prev parent reply other threads:[~2003-03-02 17:55 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2003-03-02 12:14 [PATCH] Multiple & vs. && and | vs. || bugs in 2.4.20 Norbert Kiesel
2003-03-02 18:05 ` Ulrich Drepper [this message]
2003-03-02 18:25 ` Roman Zippel
2003-03-02 21:41 ` Werner Almesberger
2003-03-02 22:03 ` Richard Henderson
2003-03-03 2:03 ` Norbert Kiesel
2003-03-03 3:02 ` John Levon
2003-03-06 19:58 ` Pavel Machek
2003-03-07 18:45 ` Norbert Kiesel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3E6247F7.8060301@redhat.com \
--to=drepper@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=nkiesel@tbdnetworks.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 NNTP newsgroup(s).