From: Rich Felker <dalias@libc.org>
To: Jaydeep Patil <Jaydeep.Patil@imgtec.com>
Cc: "musl@lists.openwall.com" <musl@lists.openwall.com>
Subject: Re: [PATCH] Fix atomic_arch.h for MIPS32 R6
Date: Tue, 29 Mar 2016 09:32:54 -0400 [thread overview]
Message-ID: <20160329133254.GM21636@brightrain.aerifal.cx> (raw)
In-Reply-To: <BD7773622145634B952E5B54ACA8E349AA24BE1D@PUMAIL01.pu.imgtec.org>
On Tue, Mar 29, 2016 at 07:16:46AM +0000, Jaydeep Patil wrote:
> >-----Original Message-----
> >From: Rich Felker [mailto:dalias@aerifal.cx] On Behalf Of Rich Felker
> >Sent: 29 March 2016 AM 09:41
> >To: Jaydeep Patil
> >Cc: musl@lists.openwall.com
> >Subject: Re: [musl] [PATCH] Fix atomic_arch.h for MIPS32 R6
> >
> >On Tue, Mar 29, 2016 at 03:54:02AM +0000, Jaydeep Patil wrote:
> >> >-----Original Message-----
> >> >From: Rich Felker [mailto:dalias@aerifal.cx] On Behalf Of Rich Felker
> >> >Sent: 28 March 2016 PM 06:35
> >> >To: Jaydeep Patil
> >> >Cc: musl@lists.openwall.com
> >> >Subject: Re: [musl] [PATCH] Fix atomic_arch.h for MIPS32 R6
> >> >
> >> >On Mon, Mar 28, 2016 at 05:07:39AM +0000, Jaydeep Patil wrote:
> >> >> >> >I was just saying it makes the code less cluttered to use them
> >> >> >> >spuriously even though we don't need to:
> >> >> >> >
> >> >> >> > ".set push ; "
> >> >> >> >#if __mips_isa_rev < 6
> >> >> >> > ".set mips2 ; "
> >> >> >> >#endif
> >> >> >> > "ll %0, %1 ; .set pop"
> >> >> >> >
> >> >> >> >or similar.
> >> >> >> >
> >> >> >> >It's also not clear to me whether the "m" constraint is valid
> >> >> >> >anymore for the R6 ll/sc instructions since they take a 9-bit
> >> >> >> >offset now instead of a
> >> >> >16-bit offset.
> >> >> >> >The compiler could generate an address expression whose offset
> >> >> >> >part does not fit in 9 bits. In that case we may need to #if
> >> >> >> >the whole function (or at least the __asm__ statement)
> >> >> >> >separately rather than just
> >> >> >skipping the .set mips2....
> >> >> >> >
> >> >> >>
> >> >> >> The "m" constrain is still valid here, as the offset will be 0 in this case..
> >> >> >
> >> >> >How can you assume the offset will be 0? It's the compiler's
> >> >> >choice what to use. For instance, a_cas(&foo->bar, t, s) is likely
> >> >> >to have an offset equal to offsetof(__typeof__(foo),bar). AFAIK
> >> >> >this happens in practice with small offsets in mutex structures,
> >> >> >etc. so the bug may be unlikely to be hit, but I think it's still an incorrect-
> >constraint bug.
> >> >>
> >> >> Compiler generates appropriate LL/SC based on the offset.
> >> >> Compiler adds the offset to the base register if it does not fit 9bits.
> >> >
> >> >The compiler has no way of knowing that the operand will be used with
> >> >ll with the 9-bit offset restriction; as far as it knows, it will be
> >> >used in a normal context where a 16-bit offset is valid. I don't have
> >> >a toolchain that will target r6, but you can try the following
> >> >program which produces an offset of 4096 for loading p[1024]:
> >> >
> >> >unsigned ll1k(volatile unsigned *p)
> >> >{
> >> > unsigned val;
> >> > __asm__ __volatile__ ("ll %0, %1" : "=r"(val) : "m"(p[1024]) :
> >> >"memory" );
> >> > return val;
> >> >}
> >> >
> >> >I would expect this to produce errors at assembly time on r6.
> >> >Rich
> >>
> >> This is what compiler has generated for above function:
> >>
> >> $ gcc -c -o main.o main.c -O3 -mips32r6 -mabi=32
> >>
> >> Objdump:
> >>
> >> 00000000 <ll1k>:
> >> 0: 24821000 addiu v0,a0,4096
> >> 4: 7c420036 ll v0,0(v0)
> >> 8: d81f0000 jrc ra
> >> c: 00000000 nop
> >
> >Can you try gcc -S instead of -c (still at -O3) to produce asm output without
> >assembling it?
>
> Generated asssembly:
>
> #APP
> # 4 "test.c" 1
> ll $2, 4096($4)
> # 0 "" 2
> #NO_APP
> jrc $31
>
> Even if we set "noreorder" before LL, assembler generates addiu+ll:
>
> 00000000 <ll1k>:
> 0: 24821000 addiu v0,a0,4096
> 4: 7c420036 ll v0,0(v0)
> 8: d81f0000 jrc ra
> c: 00000000 nop
I see. I suspected the assembler was doing it. "noat", not
"noreorder", is the way to suppress things like this but I doubt even
"noat" does it since a separate temp register ("at") is not needed in
this case.
If all assembers that support R6 support this rewriting, then the ZC
constraint in gcc is really just an optimization, not strictly
necessary. We should probably check (1) whether clang's internal
assembler can do the rewriting, and (2) whether clang supports the ZC
constraint. I would prefer using ZC but I want to do whatever is more
compatible; I don't think the codegen efficiency matters a lot either
way.
Rich
next prev parent reply other threads:[~2016-03-29 13:32 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-21 6:03 Jaydeep Patil
2016-03-21 17:37 ` dalias
2016-03-22 4:58 ` Jaydeep Patil
2016-03-22 21:22 ` Rich Felker
2016-03-23 6:37 ` Jaydeep Patil
2016-03-23 15:03 ` Rich Felker
2016-03-28 5:07 ` Jaydeep Patil
2016-03-28 13:04 ` Rich Felker
2016-03-29 2:19 ` Rich Felker
2016-03-29 3:54 ` Jaydeep Patil
2016-03-29 4:10 ` Rich Felker
2016-03-29 7:16 ` Jaydeep Patil
2016-03-29 13:32 ` Rich Felker [this message]
2016-03-30 9:45 ` Jaydeep Patil
2016-03-30 14:29 ` Rich Felker
2016-03-30 15:28 ` Rich Felker
2016-03-31 5:20 ` Jaydeep Patil
2016-03-29 3:55 ` Jaydeep Patil
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=20160329133254.GM21636@brightrain.aerifal.cx \
--to=dalias@libc.org \
--cc=Jaydeep.Patil@imgtec.com \
--cc=musl@lists.openwall.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.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
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).