Re: solaris build problem with Sun compilers

Hello all,

it appears that there's a problem building postgresql with
--enable-debug on Solaris with the Sun compilers.

noisy$ /opt/spro11/SUNWspro/bin/cc -Xa -v -DSUNOS4_CC
-I../../../../src/include -g -c s_lock.c
/opt/spro11/SUNWspro/prod/bin/fbe: "/tmp/yabeAAAdBaiW5", line 484:
error: can't compute difference between symbols in different segments
Failure in /opt/spro11/SUNWspro/prod/bin/fbe, status = 0x100
Fatal Error exec'ing /opt/spro11/SUNWspro/prod/bin/fbe
cc: acomp failed for s_lock.c

It appears that explicitly adding "-xO0" to the compile statement
resolves this problem. In fact, any optimization level seems to make
this error go away which is why it's not seen in a normal build.

The core of the problem seems to be a
.size tas_dummy,(.-tas_dummy)
directive which is emitted by the compiler.

Without the any -xO specifier this directive ends up in a different
.section due to the two .section directives in the s_lock.c code.
Adding any -xO options relocates the code so that the section mismatch
isn't a problem.

This brings up the question: why are the two .section directives
there at all in s_lock.c:299?

asm(".section \"data\"");
asm(".section \"text\"");

I don't see what they add and they're confusing assembler (fbe
actually). The data section obviously isn't needed and given this code
is already nested in a function, the text section is redundant too.

Anyway, seems we have two solutions:

1) explicitly specify a -xO0 option along with -g
or
2) remove the .section specifiers

Comments?

-- Alan

Alan Stange <stange(at)rentec(dot)com> writes:
> it appears that there's a problem building postgresql with
> --enable-debug on Solaris with the Sun compilers.

Nobody else has complained of this, so the least you could do is
identify which Solaris version and exactly which compiler you're
talking about. What are the risks that we break every other
Solaris version/compiler if we remove those directives?

regards, tom lane

Tom Lane wrote:
> Alan Stange <stange(at)rentec(dot)com> writes:
>> it appears that there's a problem building postgresql with
>> --enable-debug on Solaris with the Sun compilers.
>
> Nobody else has complained of this, so the least you could do is
> identify which Solaris version and exactly which compiler you're
> talking about.

I just tried building on all of these combinations:

Solaris 10: compilers 6.2 and 11
Solaris 9: compilers 8, 9, 10, 11
Solaris 8: compilers 6.2, 9, 11

Postgresql 8.1.3 fails to compile on all of them with --enable-debug

I can run this back to older compilers as well, if needed.

> What are the risks that we break every other
> Solaris version/compiler if we remove those directives?

none that I'm aware of.

It's been a decade since I've done any serious sparc assembler, but my
first thought on seeing this C code was that the .section directives
didn't belong. Perhaps they might have been needed on SunOS4 systems,
but I don't think it was.

Either way, add an -xO0 with --enable-debug if you really want to keep
the directives, but that's just masking the underlying problem.

-- Alan

Alan Stange <stange(at)rentec(dot)com> writes:
> Tom Lane wrote:
>> Nobody else has complained of this, so the least you could do is
>> identify which Solaris version and exactly which compiler you're
>> talking about.

> I just tried building on all of these combinations:

> Solaris 10: compilers 6.2 and 11
> Solaris 9: compilers 8, 9, 10, 11
> Solaris 8: compilers 6.2, 9, 11

> Postgresql 8.1.3 fails to compile on all of them with --enable-debug

OK, that's a reasonably convincing sample ;-). Will fix. Thanks
for the report!

regards, tom lane

Tom Lane wrote:
> Alan Stange <stange(at)rentec(dot)com> writes:
> > Tom Lane wrote:
> >> Nobody else has complained of this, so the least you could do is
> >> identify which Solaris version and exactly which compiler you're
> >> talking about.
>
> > I just tried building on all of these combinations:
>
> > Solaris 10: compilers 6.2 and 11
> > Solaris 9: compilers 8, 9, 10, 11
> > Solaris 8: compilers 6.2, 9, 11
>
> > Postgresql 8.1.3 fails to compile on all of them with --enable-debug
>
> OK, that's a reasonably convincing sample ;-). Will fix. Thanks
> for the report!

Uh, backend/port/tas/solaris*.s ASM files have "section" like:

.section ".text"

Are these OK? I didn't see you report any problems with these.

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Uh, backend/port/tas/solaris*.s ASM files have "section" like:
> .section ".text"
> Are these OK? I didn't see you report any problems with these.

Well, that's an interesting point ... I wonder what the version in
s_lock.c is used for at all? Maybe it's dead code.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Uh, backend/port/tas/solaris*.s ASM files have "section" like:
> > .section ".text"
> > Are these OK? I didn't see you report any problems with these.
>
> Well, that's an interesting point ... I wonder what the version in
> s_lock.c is used for at all? Maybe it's dead code.

Uh, I am kind of confused myself. I thought it was calling the ASM
file, but there is no x86 there so that can't be true.

I think the big question is what uses tas_dummy(). I only see it
referenced in s_lock.c.

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> Well, that's an interesting point ... I wonder what the version in
>> s_lock.c is used for at all? Maybe it's dead code.

> I think the big question is what uses tas_dummy(). I only see it
> referenced in s_lock.c.

tas_dummy, per se, isn't used at all. Notice it's declared static
so it couldn't possibly be referenced from anywhere else. The actually
exported symbols are the labels declared in the assembly code.

And the thing that is striking is that while the other two tas_dummy asm
sections make a point of saying .global or equivalent, there isn't any
such declaration in this one. Which is why I'm thinking it's dead
code. Unless the sparc assembler has the unlikely convention that
symbols are global by default, there's no way to reference this.

Alan, could you try removing the __sparc code entirely from s_lock.c
and see if it still works on all those Solaris versions?

regards, tom lane

Bruce Momjian wrote:
> Tom Lane wrote:
>
>> Alan Stange <stange(at)rentec(dot)com> writes:
>>
>>> Tom Lane wrote:
>>>
>>>> Nobody else has complained of this, so the least you could do is
>>>> identify which Solaris version and exactly which compiler you're
>>>> talking about.
>>>>
>>> I just tried building on all of these combinations:
>>>
>>> Solaris 10: compilers 6.2 and 11
>>> Solaris 9: compilers 8, 9, 10, 11
>>> Solaris 8: compilers 6.2, 9, 11
>>>
>>> Postgresql 8.1.3 fails to compile on all of them with --enable-debug
>>>
>> OK, that's a reasonably convincing sample ;-). Will fix. Thanks
>> for the report!
>>
>
> Uh, backend/port/tas/solaris*.s ASM files have "section" like:
>
> .section ".text"
>
> Are these OK? I didn't see you report any problems with these.
>
Without looking at the code (I'm home now) the main difference is that
the .s file will somewhere need a .text section. The issue in a .c file
is that one is already in the context of a .text section given that one
is in the tas_dummy() {} basic block. The .data and .text sections in
the embedded asm code is then just confusing the assembler. When the
optimizer is turned on, the .sections are possibly relocated, which
allows the fbe backend to successfully compute the .size value.

I'll remove the _tas: assembler code from the s_lock.c file and test if
the code compiles/runs on one or two solaris+compiler combinations.
I'll certainly compile on all again just to be sure.

-- Alan

Hello,

I removed the whole tas_dummy() block for sparc from s_lock.c. I
recompiled and installed with and without --enable-debug. I then ran
pgbench on the box. This is using the Sun 11 compilers on Solaris 10.

I tested using pgbench

$ pgbench -i -s 32 pgbench
$ pgbench -s 32 -c 1 -t 10000 -v

for up to 8 clients on an 8 core UltraSparc IV+ system.

Everything compiled and ran fine.

I took a look at backend/port/tas/solaris_sparc.s file. Isn't that code
a bit longer than needed?

Aren't the callers of this doing
while (tas(lock)) {
// wait a bit without smashing the kernel/memory bus
}

in which case the code should just be

ldstub [%o0],%o0
retl
nop

without the extra comparisons and branching. Unless I'm missing something.

-- Alan

Alan Stange <stange(at)rentec(dot)com> writes:
> I removed the whole tas_dummy() block for sparc from s_lock.c. I
> recompiled and installed with and without --enable-debug. I then ran
> pgbench on the box. This is using the Sun 11 compilers on Solaris 10.
> Everything compiled and ran fine.

Kinda what I thought ... okay, we can remove the entire thing then.

> I took a look at backend/port/tas/solaris_sparc.s file. Isn't that code
> a bit longer than needed?

It's already been rewritten in HEAD; uses "cas" now.

regards, tom lane

Tom Lane wrote:
> Alan Stange <stange(at)rentec(dot)com> writes:
>> I took a look at backend/port/tas/solaris_sparc.s file. Isn't that code
>> a bit longer than needed?
>
> It's already been rewritten in HEAD; uses "cas" now.

Hmmm. I've just been looking at the last snapshot of the HEAD and
s_lock.h is still using an ldstub instruction instead of a cas for the
inlined tas() function when gcc is being used. Having a cas
instruction here would probably be an improvement too, right?

Finally, I noticed that pg_sleep is calling select() for a sleep. On
Solaris, this is a fairly expensive way to get off the run queue
compared to just calling nanosleep(). How often do backends go to
sleep here under "typical" workloads? Yes, I know this is a somewhat
crazy question, I'm just trying to get a sense of how "hot" this code
path is.

Thanks!

-- Alan

Alan Stange <stange(at)rentec(dot)com> writes:
> Hmmm. I've just been looking at the last snapshot of the HEAD and
> s_lock.h is still using an ldstub instruction instead of a cas for the
> inlined tas() function when gcc is being used. Having a cas
> instruction here would probably be an improvement too, right?

[ shrug... ] The person who submitted the solaris_sparc.s change failed
to provide any evidence that it was anything but cosmetic, so I didn't
worry about changing the equivalent gcc code. If there's actually a
performance win, please cite chapter and verse. Also, shouldn't we be
worrying about breaking older Sparc chips? Does CAS go all the way
back?

> Finally, I noticed that pg_sleep is calling select() for a sleep. On
> Solaris, this is a fairly expensive way to get off the run queue
> compared to just calling nanosleep(). How often do backends go to
> sleep here under "typical" workloads?

If you actually reach that syscall you're screwed performance-wise
anyhow. I'm disinclined to mess with OS-specific variants of the
delay logic.

regards, tom lane

Tom Lane wrote:
> Alan Stange <stange(at)rentec(dot)com> writes:
> > Hmmm. I've just been looking at the last snapshot of the HEAD and
> > s_lock.h is still using an ldstub instruction instead of a cas for the
> > inlined tas() function when gcc is being used. Having a cas
> > instruction here would probably be an improvement too, right?
>
> [ shrug... ] The person who submitted the solaris_sparc.s change failed
> to provide any evidence that it was anything but cosmetic, so I didn't
> worry about changing the equivalent gcc code. If there's actually a
> performance win, please cite chapter and verse. Also, shouldn't we be
> worrying about breaking older Sparc chips? Does CAS go all the way
> back?

I don't think it is a good idea to be using different ASM instructions
based for different compilers --- they should be the same.

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> [ shrug... ] The person who submitted the solaris_sparc.s change failed
>> to provide any evidence that it was anything but cosmetic, so I didn't
>> worry about changing the equivalent gcc code. If there's actually a
>> performance win, please cite chapter and verse. Also, shouldn't we be
>> worrying about breaking older Sparc chips? Does CAS go all the way
>> back?

> I don't think it is a good idea to be using different ASM instructions
> based for different compilers --- they should be the same.

Yeah, and if I'd been applying the patch, the sparc version would have
stayed at ldstub. Like I say, there was no compelling evidence offered
for changing it.

regards, tom lane

Tom Lane wrote:
> Alan Stange <stange(at)rentec(dot)com> writes:
>> Hmmm. I've just been looking at the last snapshot of the HEAD and
>> s_lock.h is still using an ldstub instruction instead of a cas for the
>> inlined tas() function when gcc is being used. Having a cas
>> instruction here would probably be an improvement too, right?
>
> [ shrug... ] The person who submitted the solaris_sparc.s change failed
> to provide any evidence that it was anything but cosmetic, so I didn't
> worry about changing the equivalent gcc code. If there's actually a
> performance win, please cite chapter and verse. Also, shouldn't we be
> worrying about breaking older Sparc chips? Does CAS go all the way
> back?

Well, in principle the cas variant is better as fewer memory writes
occur. I'm unable to cite chapter and verse on this detail, but it is
"better".

I'm fairly sure CAS was included in the v9 instruction set. I was
actually surprised to see that you had allowed it in just for this
reason. I'm fairly sure this will break any program that needs to link
against the postgresql libs using a "v8" 32 bit version.

Either way, my original observation is that you ought to have the same
thing in either location. I'd leave the ldstub version (minus the
useless compare and assignment instructions) until you're sure everyone
is running v8plusb or v9 binaries.

Not that it matters here and now, but Solaris 10 includes atomic_ops in
libc which are probably all inlined when using the Sun compiler. It's
just what you want here and would hide most of these details. I believe
these were also added in a Solaris 9 update.
See "man atomic_ops" for details.

>> Finally, I noticed that pg_sleep is calling select() for a sleep. On
>> Solaris, this is a fairly expensive way to get off the run queue
>> compared to just calling nanosleep(). How often do backends go to
>> sleep here under "typical" workloads?
>
> If you actually reach that syscall you're screwed performance-wise
> anyhow. I'm disinclined to mess with OS-specific variants of the
> delay logic.

Not a problem. I was just poking around and asking questions.

-- Alan

Alan Stange <stange(at)rentec(dot)com> writes:
> I'm fairly sure CAS was included in the v9 instruction set. I was
> actually surprised to see that you had allowed it in just for this
> reason. I'm fairly sure this will break any program that needs to link
> against the postgresql libs using a "v8" 32 bit version.

I brought up exactly this question when the patch was submitted, and as
far as I recall it was just brushed aside. Can you tell us anything
about whether v8 chips are still in use in the field? I'm more than
ready to revert to ldstub if there's any significant risk of breaking
real users.

regards, tom lane

Tom Lane wrote:
> Alan Stange <stange(at)rentec(dot)com> writes:
>> I'm fairly sure CAS was included in the v9 instruction set. I was
>> actually surprised to see that you had allowed it in just for this
>> reason. I'm fairly sure this will break any program that needs to link
>> against the postgresql libs using a "v8" 32 bit version.
>
> I brought up exactly this question when the patch was submitted, and as
> far as I recall it was just brushed aside. Can you tell us anything
> about whether v8 chips are still in use in the field? I'm more than
> ready to revert to ldstub if there's any significant risk of breaking
> real users.

I think the vast majority of hardware in active use will be based on
"v9" instruction set hardware (meaning UltraSparc, Sparc64, etc.).
Someone running postgresql on an old sparc5 would be best off spending
$50 and getting something 10x faster from Ebay.

So, one can assume the hardware can all run "v8plus" or "v9", but the
hurdle is that some compilers will generate "generic" or "v8" binaries
still. That may be the reason why the "cas" variant wasn't comitted for
the gcc compiler as the "cas" instructions are illegal on a v8 platform.

If I try to compile the snapshot with the v62 compiler it fails:

cc -Xa -v -DSUNOS4_CC -O -c tas_cpp.s -o tas.o
/opt/spro62/SUNWspro/bin/../WS6U2/bin/fbe: "tas_cpp.s", line 1: error:
statement syntax
/opt/spro62/SUNWspro/bin/../WS6U2/bin/fbe: "tas_cpp.s", line 2: error:
unknown opcode "solaris_sparc.s"
/opt/spro62/SUNWspro/bin/../WS6U2/bin/fbe: "tas_cpp.s", line 2: error:
statement syntax
/opt/spro62/SUNWspro/bin/../WS6U2/bin/fbe: "tas_cpp.s", line 3: error:
statement syntax
/opt/spro62/SUNWspro/bin/../WS6U2/bin/fbe: "tas_cpp.s", line 13: error:
cannot use v8plus instructions in a non-v8plus target binary
cc: assembler failed for tas_cpp.s

as the default target on the older compiler is "generic". From the
spro9 compiler up to spro11 the default platform is "v8plus", for which
the cas instruction would be valid.

I modified the code that gcc would generate to use a cas instruction and
the build failed because "gcc -mv8plus" makes a call to gas using
"-xarch=v8". Needless to say, it crapped out. I was using gcc-4.0.2.

Take aways:
- postgresql HEAD when using the Solaris compilers spro9 or newer will
only run on v9 based hardware using v8plus or v9 platform.

- postgresql HEAD when using gcc will run on anything as they generate
code for the v7 platform by default and the cas instruction isn't used
in any assembler code in postgresql.

-- Alan

Alan Stange <stange(at)rentec(dot)com> writes:
> - postgresql HEAD when using the Solaris compilers spro9 or newer will
> only run on v9 based hardware using v8plus or v9 platform.
> - postgresql HEAD when using gcc will run on anything as they generate
> code for the v7 platform by default and the cas instruction isn't used
> in any assembler code in postgresql.

I gather from the contents of solaris_sparc.s that the Sun compilers can
be expected to define __sparcv9 if compiling for v9 hardware. I'm
inclined to #ifdef things so that we use cas if that symbol's defined and
ldstub if not. I'm not sure if gcc can be expected to define the same
symbol --- we might end up using ldstub always, even if we try to
conditionalize the code for gcc.

Or we could just revert to ldstub. This seems like a lot of complexity
for a so-far-entirely-hypothetical performance gain ...

regards, tom lane

Tom Lane wrote:
> Alan Stange <stange(at)rentec(dot)com> writes:
>> - postgresql HEAD when using the Solaris compilers spro9 or newer will
>> only run on v9 based hardware using v8plus or v9 platform.
>> - postgresql HEAD when using gcc will run on anything as they generate
>> code for the v7 platform by default and the cas instruction isn't used
>> in any assembler code in postgresql.
>
> I gather from the contents of solaris_sparc.s that the Sun compilers can
> be expected to define __sparcv9 if compiling for v9 hardware. I'm
> inclined to #ifdef things so that we use cas if that symbol's defined and
> ldstub if not. I'm not sure if gcc can be expected to define the same
> symbol --- we might end up using ldstub always, even if we try to
> conditionalize the code for gcc.

This code isn't used by gcc, is it? Or are you thinking to use the same
assembler segment in the inline function used by gcc?

> Or we could just revert to ldstub. This seems like a lot of complexity
> for a so-far-entirely-hypothetical performance gain ...

I suspect that on machines like the UltraSparc T1 (the multi-core boxes)
this can have an impact (lots of additional memory writes). Even so,
that seems like a stretch.

Was there any reason given for the cas patch?

I might get around to doing the work for the Solaris compiler build to
support the inline keyword as well. I hacked on it a bit today but had
to get back to real work...

-- Alan

Alan Stange <stange(at)rentec(dot)com> writes:
> This code isn't used by gcc, is it? Or are you thinking to use the same
> assembler segment in the inline function used by gcc?

The out-of-line .s files aren't used for gcc builds --- what's on the
table here is whether to make the gcc inline assembler in s_lock.h
match up with what's in the out-of-line files.

> Was there any reason given for the cas patch?

It appeared to me that the submitter just had a bee in his bonnet about
cas being nicer ;-). There might be something real to it, but AFAIR he
didn't present any hard evidence.

> I might get around to doing the work for the Solaris compiler build to
> support the inline keyword as well. I hacked on it a bit today but had
> to get back to real work...

If you can find some time it'd be great. It's risky to fool with this
code without access to hardware to test it on, so I'm not very inclined
to change it myself.

regards, tom lane

Tom Lane wrote:
> Alan Stange <stange(at)rentec(dot)com> writes:
>> This code isn't used by gcc, is it? Or are you thinking to use the same
>> assembler segment in the inline function used by gcc?
>
> The out-of-line .s files aren't used for gcc builds --- what's on the
> table here is whether to make the gcc inline assembler in s_lock.h
> match up with what's in the out-of-line files.
>
>> Was there any reason given for the cas patch?
>
> It appeared to me that the submitter just had a bee in his bonnet about
> cas being nicer ;-). There might be something real to it, but AFAIR he
> didn't present any hard evidence.

Here's one workaround.

Check out the comment and implementation for cas32() in this .il
template file for libc from OpenSolaris:

http://cvs.opensolaris.org/source/xref/on/usr/src/lib/libc/sparc/threads/sparc.il

Alan Stange <stange(at)rentec(dot)com> writes:
> Check out the comment and implementation for cas32() in this .il
> template file for libc from OpenSolaris:

> http://cvs.opensolaris.org/source/xref/on/usr/src/lib/libc/sparc/threads/sparc.il

If you mean

* When compiling with -xarch=v8, the compiler refuses to
* accept the 'cas' instruction, so we encode it in hex below.

I can't say that that impresses me. It still will fail on v8 chips no?
What's the point of fooling the compiler if you can't fool the hardware?

regards, tom lane

Tom Lane wrote:
> Alan Stange <stange(at)rentec(dot)com> writes:
> > Check out the comment and implementation for cas32() in this .il
> > template file for libc from OpenSolaris:
>
> > http://cvs.opensolaris.org/source/xref/on/usr/src/lib/libc/sparc/threads/sparc.il
>
> If you mean
>
> * When compiling with -xarch=v8, the compiler refuses to
> * accept the 'cas' instruction, so we encode it in hex below.
>
> I can't say that that impresses me. It still will fail on v8 chips no?
> What's the point of fooling the compiler if you can't fool the hardware?

Wow, that is _insane_ --- you can't code the ASM, so you specify the
instructions in _hex_. However, they must like cas more than ldstub or
they wouldn't have gone through that much work to use cas.

I am thinking we revert to ldstub and add a comment that we are not
using cas() because of sparc v8.

I just checked gcc on Solaris 2.9 (Solaris 9?), and there is no
specification of the type of sparc processor:

/usr/local/lib/gcc-lib/sparc-sun-solaris2.9/3.3.2/cc1 -quiet -v
-D__GNUC__=3 -D__GNUC_MINOR__=3 -D__GNUC_PATCHLEVEL__=2 -Dsparc
-D__sparc__ -D__sparc -D__GCC_NEW_VARARGS__ -Acpu=sparc -Amachine=sparc
x.c -quiet -dumpbase x.c -auxbase x -version -o /var/tmp//ccBSywG7.s

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:
> Alan Stange <stange(at)rentec(dot)com> writes:
>
>> Check out the comment and implementation for cas32() in this .il
>> template file for libc from OpenSolaris:
>>
>
>
>> http://cvs.opensolaris.org/source/xref/on/usr/src/lib/libc/sparc/threads/sparc.il
>>
>
> If you mean
>
> * When compiling with -xarch=v8, the compiler refuses to
> * accept the 'cas' instruction, so we encode it in hex below.
>
> I can't say that that impresses me. It still will fail on v8 chips no?
> What's the point of fooling the compiler if you can't fool the hardware?
I believe the trick here is that Solaris 10 will only run on v9 hardware
(or the sun4u systems), which all have the instruction. But the v8 ABI
"model" doesn't have it. So, in some sense the ABI doesn't "allow"
this instruction, but the hardware does, so they can just slam it in
knowing that it'll work.

-- Alan

Alan Stange <stange(at)rentec(dot)com> writes:
> I believe the trick here is that Solaris 10 will only run on v9 hardware
> (or the sun4u systems), which all have the instruction. But the v8 ABI
> "model" doesn't have it. So, in some sense the ABI doesn't "allow"
> this instruction, but the hardware does, so they can just slam it in
> knowing that it'll work.

Well, that might be OK for Sun's compiler since they know what a given
version of Solaris will run on. But I don't think we can adopt the same
attitude for our gcc code path; that has to work on Sparc-based Linuxen
and BSDen. I don't think it's appropriate to kiss off support for v8
chips when we haven't seen any proof at all of a performance boost in
return.

Maybe the right answer is to leave the code as-is, ie, deliberately not
the same for Sun and gcc compilers.

regards, tom lane

Alan Stange wrote:
> Tom Lane wrote:
> > Alan Stange <stange(at)rentec(dot)com> writes:
> >
> >> Check out the comment and implementation for cas32() in this .il
> >> template file for libc from OpenSolaris:
> >>
> >
> >
> >> http://cvs.opensolaris.org/source/xref/on/usr/src/lib/libc/sparc/threads/sparc.il
> >>
> >
> > If you mean
> >
> > * When compiling with -xarch=v8, the compiler refuses to
> > * accept the 'cas' instruction, so we encode it in hex below.
> >
> > I can't say that that impresses me. It still will fail on v8 chips no?
> > What's the point of fooling the compiler if you can't fool the hardware?
> I believe the trick here is that Solaris 10 will only run on v9 hardware
> (or the sun4u systems), which all have the instruction. But the v8 ABI
> "model" doesn't have it. So, in some sense the ABI doesn't "allow"
> this instruction, but the hardware does, so they can just slam it in
> knowing that it'll work.

Uh, are you saying our Solaris Sparc "cas" ASM is only going to work for
Solaris 11 tools? That doesn't sound good. We must have people using
earlier Solaris tools.

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:
> Alan Stange <stange(at)rentec(dot)com> writes:
> > I believe the trick here is that Solaris 10 will only run on v9 hardware
> > (or the sun4u systems), which all have the instruction. But the v8 ABI
> > "model" doesn't have it. So, in some sense the ABI doesn't "allow"
> > this instruction, but the hardware does, so they can just slam it in
> > knowing that it'll work.
>
> Well, that might be OK for Sun's compiler since they know what a given
> version of Solaris will run on. But I don't think we can adopt the same
> attitude for our gcc code path; that has to work on Sparc-based Linuxen
> and BSDen. I don't think it's appropriate to kiss off support for v8
> chips when we haven't seen any proof at all of a performance boost in
> return.
>
> Maybe the right answer is to leave the code as-is, ie, deliberately not
> the same for Sun and gcc compilers.

OK, I have applied the following patch which documents that the Solaris
CC TAS ASM only works for sparc9. Hopefully either sparc8 is not needed
(unlikely) or someone will fix it. I have CC'ed the original
contributor who added "cas".

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Tom Lane wrote:
> > Alan Stange <stange(at)rentec(dot)com> writes:
> > > I believe the trick here is that Solaris 10 will only run on v9 hardware
> > > (or the sun4u systems), which all have the instruction. But the v8 ABI
> > > "model" doesn't have it. So, in some sense the ABI doesn't "allow"
> > > this instruction, but the hardware does, so they can just slam it in
> > > knowing that it'll work.
> >
> > Well, that might be OK for Sun's compiler since they know what a given
> > version of Solaris will run on. But I don't think we can adopt the same
> > attitude for our gcc code path; that has to work on Sparc-based Linuxen
> > and BSDen. I don't think it's appropriate to kiss off support for v8
> > chips when we haven't seen any proof at all of a performance boost in
> > return.
> >
> > Maybe the right answer is to leave the code as-is, ie, deliberately not
> > the same for Sun and gcc compilers.
>
> OK, I have applied the following patch which documents that the Solaris
> CC TAS ASM only works for sparc9. Hopefully either sparc8 is not needed
> (unlikely) or someone will fix it. I have CC'ed the original
> contributor who added "cas".

Oh, I just found this email that has a simplified sparc8 asm:

http://archives.postgresql.org/pgsql-ports/2006-05/msg00025.php

Attached is the new solaris_sparc.s file with the #ifdef sparc8 test;
applied.

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Alan Stange wrote:
> Hello,
>
> I removed the whole tas_dummy() block for sparc from s_lock.c. I
> recompiled and installed with and without --enable-debug. I then ran
> pgbench on the box. This is using the Sun 11 compilers on Solaris 10.
>
> I tested using pgbench
>
> $ pgbench -i -s 32 pgbench
> $ pgbench -s 32 -c 1 -t 10000 -v
>
> for up to 8 clients on an 8 core UltraSparc IV+ system.
>
> Everything compiled and ran fine.
>
>
>
>
> I took a look at backend/port/tas/solaris_sparc.s file. Isn't that code
> a bit longer than needed?
>
> Aren't the callers of this doing
> while (tas(lock)) {
> // wait a bit without smashing the kernel/memory bus
> }
>
> in which case the code should just be
>
> ldstub [%o0],%o0
> retl
> nop
>
> without the extra comparisons and branching. Unless I'm missing something.

Basically the original 8.1 ASM code was placing its own address into the
shared memory address:

ldstub [%o0],%o0

and then based on what it got back, it was returning 1/0. The new code
in CVS HEAD places '1' in, so there is no longer a need for the tests.

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On May 17, 2006, at 8:04 PM, Bruce Momjian wrote:

> Bruce Momjian wrote:
>> Tom Lane wrote:
>>> Alan Stange <stange(at)rentec(dot)com> writes:
>>>> I believe the trick here is that Solaris 10 will only run on v9
>>>> hardware
>>>> (or the sun4u systems), which all have the instruction. But
>>>> the v8 ABI
>>>> "model" doesn't have it. So, in some sense the ABI doesn't
>>>> "allow"
>>>> this instruction, but the hardware does, so they can just slam
>>>> it in
>>>> knowing that it'll work.
>>>
>>> Well, that might be OK for Sun's compiler since they know what a
>>> given
>>> version of Solaris will run on. But I don't think we can adopt
>>> the same
>>> attitude for our gcc code path; that has to work on Sparc-based
>>> Linuxen
>>> and BSDen. I don't think it's appropriate to kiss off support
>>> for v8
>>> chips when we haven't seen any proof at all of a performance
>>> boost in
>>> return.
>>>
>>> Maybe the right answer is to leave the code as-is, ie,
>>> deliberately not
>>> the same for Sun and gcc compilers.
>>
>> OK, I have applied the following patch which documents that the
>> Solaris
>> CC TAS ASM only works for sparc9. Hopefully either sparc8 is not
>> needed
>> (unlikely) or someone will fix it. I have CC'ed the original
>> contributor who added "cas".
>
> Oh, I just found this email that has a simplified sparc8 asm:
>
> http://archives.postgresql.org/pgsql-ports/2006-05/msg00025.php
>
> Attached is the new solaris_sparc.s file with the #ifdef sparc8 test;
> applied.

I don't think that asm does what you think it does. That hex
encoding of the cas instruction doesn't work on Sparcv8, only
sparcv8plus. The reason that it is hacked that way is that, for
other reasons, they can't use the -xarch=v8plus flag (despite
compiling on v8plus capable chips). Basically that code hardcodes a
v8plus instruction into a v8 binary "knowing" that it will never run
on a non-v8plus capable chip. The reason that they can do this is
because they (as I understand it) open solaris won't support any
chips so old as to not run v8plus code.

We shouldn't be suffering from that problem and -xarch=v8plus should
be used to produce 32-bit binaries and -xarch=v9 should be used to
produce 64-bit sparc binaries. I can't think of a reason to every
compile the Postgres source with -xarch=v8 instead of -xarch=v8plus,
the latter will produce much better code overall.

// Theo Schlossnagle
// CTO -- http://www.omniti.com/~jesus/
// OmniTI Computer Consulting, Inc. -- http://www.omniti.com/
// Ecelerity: Run with it.

Theo Schlossnagle wrote:
> > Oh, I just found this email that has a simplified sparc8 asm:
> >
> > http://archives.postgresql.org/pgsql-ports/2006-05/msg00025.php
> >
> > Attached is the new solaris_sparc.s file with the #ifdef sparc8 test;
> > applied.
>
> I don't think that asm does what you think it does. That hex
> encoding of the cas instruction doesn't work on Sparcv8, only
> sparcv8plus. The reason that it is hacked that way is that, for
> other reasons, they can't use the -xarch=v8plus flag (despite
> compiling on v8plus capable chips). Basically that code hardcodes a
> v8plus instruction into a v8 binary "knowing" that it will never run
> on a non-v8plus capable chip. The reason that they can do this is
> because they (as I understand it) open solaris won't support any
> chips so old as to not run v8plus code.
>
> We shouldn't be suffering from that problem and -xarch=v8plus should
> be used to produce 32-bit binaries and -xarch=v9 should be used to
> produce 64-bit sparc binaries. I can't think of a reason to every
> compile the Postgres source with -xarch=v8 instead of -xarch=v8plus,
> the latter will produce much better code overall.

OK, so what do you suggest? Remove that URL from the comments?
Anything else?

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On May 17, 2006, at 8:30 PM, Bruce Momjian wrote:

> Theo Schlossnagle wrote:
>>> Oh, I just found this email that has a simplified sparc8 asm:
>>>
>>> http://archives.postgresql.org/pgsql-ports/2006-05/msg00025.php
>>>
>>> Attached is the new solaris_sparc.s file with the #ifdef sparc8
>>> test;
>>> applied.
>>
>> I don't think that asm does what you think it does. That hex
>> encoding of the cas instruction doesn't work on Sparcv8, only
>> sparcv8plus. The reason that it is hacked that way is that, for
>> other reasons, they can't use the -xarch=v8plus flag (despite
>> compiling on v8plus capable chips). Basically that code hardcodes a
>> v8plus instruction into a v8 binary "knowing" that it will never run
>> on a non-v8plus capable chip. The reason that they can do this is
>> because they (as I understand it) open solaris won't support any
>> chips so old as to not run v8plus code.
>>
>> We shouldn't be suffering from that problem and -xarch=v8plus should
>> be used to produce 32-bit binaries and -xarch=v9 should be used to
>> produce 64-bit sparc binaries. I can't think of a reason to every
>> compile the Postgres source with -xarch=v8 instead of -xarch=v8plus,
>> the latter will produce much better code overall.
>
> OK, so what do you suggest? Remove that URL from the comments?
> Anything else?

If you compile with -xarch=v8plus you should be able to omit the
#ifdef's entirely. The sparcv8plus and sparcv9 cas instruction are
identical. Is the goal here to support Sparc chips that do not
support the v8plus instruction set? I'm not on the ports list, so
maybe I missed the original problem report.

// Theo Schlossnagle
// CTO -- http://www.omniti.com/~jesus/
// OmniTI Computer Consulting, Inc. -- http://www.omniti.com/
// Ecelerity: Run with it.

Theo Schlossnagle wrote:
>
> On May 17, 2006, at 8:30 PM, Bruce Momjian wrote:
>
> > Theo Schlossnagle wrote:
> >>> Oh, I just found this email that has a simplified sparc8 asm:
> >>>
> >>> http://archives.postgresql.org/pgsql-ports/2006-05/msg00025.php
> >>>
> >>> Attached is the new solaris_sparc.s file with the #ifdef sparc8
> >>> test;
> >>> applied.
> >>
> >> I don't think that asm does what you think it does. That hex
> >> encoding of the cas instruction doesn't work on Sparcv8, only
> >> sparcv8plus. The reason that it is hacked that way is that, for
> >> other reasons, they can't use the -xarch=v8plus flag (despite
> >> compiling on v8plus capable chips). Basically that code hardcodes a
> >> v8plus instruction into a v8 binary "knowing" that it will never run
> >> on a non-v8plus capable chip. The reason that they can do this is
> >> because they (as I understand it) open solaris won't support any
> >> chips so old as to not run v8plus code.
> >>
> >> We shouldn't be suffering from that problem and -xarch=v8plus should
> >> be used to produce 32-bit binaries and -xarch=v9 should be used to
> >> produce 64-bit sparc binaries. I can't think of a reason to every
> >> compile the Postgres source with -xarch=v8 instead of -xarch=v8plus,
> >> the latter will produce much better code overall.
> >
> > OK, so what do you suggest? Remove that URL from the comments?
> > Anything else?
>
> If you compile with -xarch=v8plus you should be able to omit the
> #ifdef's entirely. The sparcv8plus and sparcv9 cas instruction are
> identical. Is the goal here to support Sparc chips that do not
> support the v8plus instruction set? I'm not on the ports list, so
> maybe I missed the original problem report.

Uh, we have no idea what cpus people are using, nor what compile flags
that use. People are probably going to use whatever their compiler
defaults are, which I am afraid is often sparcv8.

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Alan Stange wrote:
>> Tom Lane wrote:
>>> Alan Stange <stange(at)rentec(dot)com> writes:
>>>
>>>> Check out the comment and implementation for cas32() in this .il
>>>> template file for libc from OpenSolaris:
>>>>
>>>
>>>> http://cvs.opensolaris.org/source/xref/on/usr/src/lib/libc/sparc/threads/sparc.il
>>>>
>>> If you mean
>>>
>>> * When compiling with -xarch=v8, the compiler refuses to
>>> * accept the 'cas' instruction, so we encode it in hex below.
>>>
>>> I can't say that that impresses me. It still will fail on v8 chips no?
>>> What's the point of fooling the compiler if you can't fool the hardware?
>> I believe the trick here is that Solaris 10 will only run on v9 hardware
>> (or the sun4u systems), which all have the instruction. But the v8 ABI
>> "model" doesn't have it. So, in some sense the ABI doesn't "allow"
>> this instruction, but the hardware does, so they can just slam it in
>> knowing that it'll work.

> Uh, are you saying our Solaris Sparc "cas" ASM is only going to work for
> Solaris 11 tools? That doesn't sound good. We must have people using
> earlier Solaris tools.

No, I'm not saying that at all.

I think I'm saying that cas is OK for any reasonable Solaris+Sparc+Sun
compiler combination. It'll fail on any SuperSparc based hardware and
older compilers (more than ~5 years), but that's too bad.

I'm also saying that cas in not OK for any gcc+binutils combination. As
far as I can tell, gcc is still building code for the v7 platform by
default. It's possible to override this, but I don't think it's
something you can test for in the preprocessor.

-- Alan

Theo Schlossnagle wrote:
>
> On May 17, 2006, at 8:04 PM, Bruce Momjian wrote:
>
>> Bruce Momjian wrote:
>>> Tom Lane wrote:
>>>> Alan Stange <stange(at)rentec(dot)com> writes:
>>>>> I believe the trick here is that Solaris 10 will only run on v9
>>>>> hardware
>>>>> (or the sun4u systems), which all have the instruction. But the
>>>>> v8 ABI
>>>>> "model" doesn't have it. So, in some sense the ABI doesn't "allow"
>>>>> this instruction, but the hardware does, so they can just slam it in
>>>>> knowing that it'll work.
>>>>
>>>> Well, that might be OK for Sun's compiler since they know what a given
>>>> version of Solaris will run on. But I don't think we can adopt the
>>>> same
>>>> attitude for our gcc code path; that has to work on Sparc-based Linuxen
>>>> and BSDen. I don't think it's appropriate to kiss off support for v8
>>>> chips when we haven't seen any proof at all of a performance boost in
>>>> return.
>>>>
>>>> Maybe the right answer is to leave the code as-is, ie, deliberately not
>>>> the same for Sun and gcc compilers.
>>>
>>> OK, I have applied the following patch which documents that the Solaris
>>> CC TAS ASM only works for sparc9. Hopefully either sparc8 is not needed
>>> (unlikely) or someone will fix it. I have CC'ed the original
>>> contributor who added "cas".
>>
>> Oh, I just found this email that has a simplified sparc8 asm:
>>
>> http://archives.postgresql.org/pgsql-ports/2006-05/msg00025.php
>>
>> Attached is the new solaris_sparc.s file with the #ifdef sparc8 test;
>> applied.
>
> I don't think that asm does what you think it does. That hex encoding
> of the cas instruction doesn't work on Sparcv8, only sparcv8plus. The
> reason that it is hacked that way is that, for other reasons, they can't
> use the -xarch=v8plus flag (despite compiling on v8plus capable chips).
> Basically that code hardcodes a v8plus instruction into a v8 binary
> "knowing" that it will never run on a non-v8plus capable chip. The
> reason that they can do this is because they (as I understand it) open
> solaris won't support any chips so old as to not run v8plus code.

I think that's what was written above...

> We shouldn't be suffering from that problem and -xarch=v8plus should be
> used to produce 32-bit binaries and -xarch=v9 should be used to produce
> 64-bit sparc binaries. I can't think of a reason to every compile the
> Postgres source with -xarch=v8 instead of -xarch=v8plus, the latter will
> produce much better code overall.

linking the postgresql libpq with code compiled at v8 is the only reason
that I can think of. v8plus has been the default in the Sun compilers
for some time now, so this would only be an issue for an older binary.
As far as I can tell, gcc still produces v7 code by default and knows
nothing about v8plus.

Even better would be using v8plusb with the Sun compilers.

-- Alan

Alan Stange wrote:
> > Uh, are you saying our Solaris Sparc "cas" ASM is only going to work for
> > Solaris 11 tools? That doesn't sound good. We must have people using
> > earlier Solaris tools.
>
> No, I'm not saying that at all.
>
> I think I'm saying that cas is OK for any reasonable Solaris+Sparc+Sun
> compiler combination. It'll fail on any SuperSparc based hardware and
> older compilers (more than ~5 years), but that's too bad.

Well, when you look at the cost/benefit of using cas vs. ldstub, it
seems logical to keep ldstub for those older platforms.

> I'm also saying that cas in not OK for any gcc+binutils combination. As
> far as I can tell, gcc is still building code for the v7 platform by
> default. It's possible to override this, but I don't think it's
> something you can test for in the preprocessor.

OK, I added a comment that gcc generates sparcv7 binaries.

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +