This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] hppa: fix __O_SYNC to match the kernel
- From: John David Anglin <dave dot anglin at bell dot net>
- To: "Carlos O'Donell" <carlos at redhat dot com>
- Cc: Mike Frysinger <vapier at gentoo dot org>, libc-alpha at sourceware dot org, carlos at systemhalted dot org
- Date: Sun, 8 Mar 2015 17:30:14 -0400
- Subject: Re: [PATCH] hppa: fix __O_SYNC to match the kernel
- Authentication-results: sourceware.org; auth=none
- References: <1424755185-27627-1-git-send-email-vapier at gentoo dot org> <BLU436-SMTP185F8B8082D09E2C4A6E5EF97160 at phx dot gbl> <20150227065339 dot GO29461 at vapier> <BLU436-SMTP175B7131A983C54990446FB97130 at phx dot gbl> <54FCA873 dot 3030008 at redhat dot com>
On 2015-03-08, at 3:52 PM, Carlos O'Donell wrote:
> On 03/01/2015 02:55 PM, John David Anglin wrote:
>> In fesetenv.c, the post increment of bufptr was retained in the first asm but the constraint for it
>> does not indicate that bufptr is modified. As a result, GCC miscompiled fesetenv. We get better
>> code by not modifying bufptr as GCC doesn't have to reload bufptr.
>
> It uses "+r" which means the operand, the register, is read and modified.
>
> The fact that we get better code, or work around a gcc bug, is a good reason
> to make the change, but I don't see what's wrong with the original code.
>
>> The main bug in feholdexcept is the second set of bufptr. This existed to the restore the exception
>> registers in reverse order. This statement should have been removed when the code was changed
>> to only update the status and first exception registers. The offset used in the statement is also off
>> by a factor two, so it probably never worked correctly. With the current patch, the code loads zero to
>> the status and exception register. As a result, the T bit is not set properly.
>
> This doesn't sound right either.
>
> fstd,ma %%fr0,8(%1)
>
> Results in a store to bufptr, with bubptr-=8 *after* the store.
>
> fldd,mb -8(%0),%%fr0
>
> Results in a bufptr-=8, followed by a load from bufptr to fr0.
>
> Thus while the operations are left-over from previous iterations of
> the code where we did save/load all of the registers, the above code
> is idempotent with respect to your changes.
>
> The one problem is that bufptr is not marked as changed in the *second*
> assembly contstraint which just lists bufptr as input (which is wrong)
> and an optimization might reorder things such that this breaks.
Exactly. The post and pre-increments could be used but one needs to ensure that the bufptr asm
arguments are marked as written. I suspect instructions with post and pre-increment are slightly
slower but the difference may not be visible given the length of the pipeline. We want to avoid
re-initializing bufptr.
Thus, I believe it's better to not modify bufptr in the asms in these two functions.
Dave
--
John David Anglin dave.anglin@bell.net