[patch 5/5] fix sn rw_mmr.h to use intrinsic

[patch 5/5] fix sn rw_mmr.h to use intrinsic

[Chen, Kenneth W]

Use ia64 intrinsic for functions in sn/rw_mmr.h.

Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>


--- ./include/asm-ia64/sn/rw_mmr.h.orig	2006-01-26 18:27:42.750607692 -0800
+++ ./include/asm-ia64/sn/rw_mmr.h	2006-01-26 18:33:46.337517301 -0800
@@ -25,50 +25,43 @@
 extern inline long
 pio_phys_read_mmr(volatile long *mmr) 
 {
-	long val;
-        asm volatile
-            ("mov r2=psr;;"
-             "rsm psr.i | psr.dt;;"
-             "srlz.i;;"
-             "ld8.acq %0=[%1];;"
-             "mov psr.l=r2;;"
-             "srlz.i;;"
-             : "=r"(val)
-             : "r"(mmr)
-	     : "r2");
-        return val;
-}
-
+	long val, flags;
 
+	local_irq_save(flags);
+	ia64_rsm(IA64_PSR_DT);
+	ia64_srlz_d();
+	val = *mmr;
+	ia64_ssm(IA64_PSR_DT);
+	local_irq_restore(flags);
+	return val;
+}
 
 extern inline void
 pio_phys_write_mmr(volatile long *mmr, long val) 
 {
-        asm volatile
-            ("mov r2=psr;;"
-             "rsm psr.i | psr.dt;;"
-             "srlz.i;;"
-             "st8.rel [%0]=%1;;"
-             "mov psr.l=r2;;"
-             "srlz.i;;"
-	     :: "r"(mmr), "r"(val)
-             : "r2", "memory");
-}            
+	unsigned long flags;
+
+	local_irq_save(flags);
+	ia64_rsm(IA64_PSR_DT);
+	ia64_srlz_d();
+	*mmr = val;
+	ia64_ssm(IA64_PSR_DT);
+	local_irq_restore(flags);
+}
 
 extern inline void
 pio_atomic_phys_write_mmrs(volatile long *mmr1, long val1, volatile long *mmr2, long val2) 
 {
-        asm volatile
-            ("mov r2=psr;;"
-             "rsm psr.i | psr.dt | psr.ic;;"
-	     "cmp.ne p9,p0=%2,r0;"
-             "srlz.i;;"
-             "st8.rel [%0]=%1;"
-             "(p9) st8.rel [%2]=%3;;"
-             "mov psr.l=r2;;"
-             "srlz.i;;"
-	     :: "r"(mmr1), "r"(val1), "r"(mmr2), "r"(val2)
-             : "p9", "r2", "memory");
-}            
+	unsigned long flags;
+
+	local_irq_save(flags);
+	ia64_rsm(IA64_PSR_DT);
+	ia64_srlz_d();
+	*mmr1 = val1;
+	if (mmr2)
+		*mmr2 = val2;
+	ia64_ssm(IA64_PSR_DT);
+	local_irq_restore(flags);
+}
 
 #endif /* _ASM_IA64_SN_RW_MMR_H */

RE: [patch 5/5] fix sn rw_mmr.h to use intrinsic

[Chen, Kenneth W]

Use ia64 intrinsic for functions in sn/rw_mmr.h.

Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>

---
Shhh.  Before anyone noticed, here is a new version that replaces
the original patch [5/5]

By the way, for my learning purpose, why does pio_atomic_phys_write_mmrs()
need to turn off psr.ic bit?


diff -Nurp linus-2.6.git/include/asm-ia64/sn/rw_mmr.h linus-2.6.git.new/include/asm-ia64/sn/rw_mmr.h
--- linus-2.6.git/include/asm-ia64/sn/rw_mmr.h	2006-01-25 16:39:25.000000000 -0800
+++ linus-2.6.git.new/include/asm-ia64/sn/rw_mmr.h	2006-01-26 20:51:14.424330324 -0800
@@ -25,50 +25,43 @@
 extern inline long
 pio_phys_read_mmr(volatile long *mmr) 
 {
-	long val;
-        asm volatile
-            ("mov r2=psr;;"
-             "rsm psr.i | psr.dt;;"
-             "srlz.i;;"
-             "ld8.acq %0=[%1];;"
-             "mov psr.l=r2;;"
-             "srlz.i;;"
-             : "=r"(val)
-             : "r"(mmr)
-	     : "r2");
-        return val;
-}
-
+	long val, flags;
 
+	flags = ia64_getreg(_IA64_REG_PSR);
+	ia64_rsm(IA64_PSR_I | IA64_PSR_DT);
+	ia64_srlz_d();
+	val = *mmr;
+	ia64_setreg(_IA64_REG_PSR, flags);
+	ia64_srlz_d();
+	return val;
+}
 
 extern inline void
 pio_phys_write_mmr(volatile long *mmr, long val) 
 {
-        asm volatile
-            ("mov r2=psr;;"
-             "rsm psr.i | psr.dt;;"
-             "srlz.i;;"
-             "st8.rel [%0]=%1;;"
-             "mov psr.l=r2;;"
-             "srlz.i;;"
-	     :: "r"(mmr), "r"(val)
-             : "r2", "memory");
-}            
+	unsigned long flags;
+
+	flags = ia64_getreg(_IA64_REG_PSR);
+	ia64_rsm(IA64_PSR_I | IA64_PSR_DT);
+	ia64_srlz_d();
+	*mmr = val;
+	ia64_setreg(_IA64_REG_PSR, flags);
+ 	ia64_srlz_d();
+}
 
 extern inline void
 pio_atomic_phys_write_mmrs(volatile long *mmr1, long val1, volatile long *mmr2, long val2) 
 {
-        asm volatile
-            ("mov r2=psr;;"
-             "rsm psr.i | psr.dt | psr.ic;;"
-	     "cmp.ne p9,p0=%2,r0;"
-             "srlz.i;;"
-             "st8.rel [%0]=%1;"
-             "(p9) st8.rel [%2]=%3;;"
-             "mov psr.l=r2;;"
-             "srlz.i;;"
-	     :: "r"(mmr1), "r"(val1), "r"(mmr2), "r"(val2)
-             : "p9", "r2", "memory");
-}            
+	unsigned long flags;
+
+	flags = ia64_getreg(_IA64_REG_PSR);
+	ia64_rsm(IA64_PSR_DT | IA64_PSR_I | IA64_PSR_IC);
+	ia64_srlz_i();
+	*mmr1 = val1;
+	if (mmr2)
+		*mmr2 = val2;
+	ia64_setreg(_IA64_REG_PSR, flags);
+ 	ia64_srlz_i();
+}
 
 #endif /* _ASM_IA64_SN_RW_MMR_H */

Re: [patch 5/5] fix sn rw_mmr.h to use intrinsic

[Jes Sorensen]

>>>>> "Kenneth" = Chen, Kenneth W <kenneth.w.chen@intel.com> writes:

Kenneth> Use ia64 intrinsic for functions in sn/rw_mmr.h.
Kenneth> Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>

Hi Ken,

I'll have to test this. However it really doesn't matter much as we
won't be building the SN kernel with ICC.

Kenneth> By the way, for my learning purpose, why does
Kenneth> pio_atomic_phys_write_mmrs() need to turn off psr.ic bit?

No idea, maybe Jack knows?

Cheers,
Jes

RE: [patch 5/5] fix sn rw_mmr.h to use intrinsic

[Chen, Kenneth W]

Jes Sorensen wrote on Friday, January 27, 2006 1:57 AM
> Kenneth> Use ia64 intrinsic for functions in sn/rw_mmr.h.
> Kenneth> Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>
> 
> I'll have to test this. However it really doesn't matter much as we
> won't be building the SN kernel with ICC.

When I build a generic kernel, all sn code are pulled in and need to
be compiled.  It would be nice to have sn portion of kernel code free
of asm statement in the source. I heard that gcc is also going in the
direction of implementing intrinsic in the long term.  Or do we have
an option of CONFIG_NON_SGI?  I can cook up something if this is really
what people Want (I suspect not).

- Ken

Re: [patch 5/5] fix sn rw_mmr.h to use intrinsic

[Jes Sorensen]

>>>>> "Ken" = Chen, Kenneth W <kenneth.w.chen@intel.com> writes:

Ken> Jes Sorensen wrote on Friday, January 27, 2006 1:57 AM
>>  I'll have to test this. However it really doesn't matter much as
>> we won't be building the SN kernel with ICC.

Ken> When I build a generic kernel, all sn code are pulled in and need
Ken> to be compiled.  It would be nice to have sn portion of kernel
Ken> code free of asm statement in the source. I heard that gcc is
Ken> also going in the direction of implementing intrinsic in the long
Ken> term.  Or do we have an option of CONFIG_NON_SGI?  I can cook up
Ken> something if this is really what people Want (I suspect not).

Ken,

Speaking solely on my own behalf, and not that of my employer here.

I have to say that I have always found it very discouraging how the
kernel had this mess added which is badly obfuscating the code solely
to support one vendor's broken compiler.

The real solution to this problem would be for Intel's compiler people
to fix the brokenness of ICC instead of adding any more of this
obfuscation to the kernel ;-(

Regards,
Jes

Re: [patch 5/5] fix sn rw_mmr.h to use intrinsic

[Christoph Hellwig]

On Tue, Jan 31, 2006 at 05:57:05AM -0500, Jes Sorensen wrote:
> I have to say that I have always found it very discouraging how the
> kernel had this mess added which is badly obfuscating the code solely
> to support one vendor's broken compiler.
> 
> The real solution to this problem would be for Intel's compiler people
> to fix the brokenness of ICC instead of adding any more of this
> obfuscation to the kernel ;-(

Agreed.  Obsfucating for a propritary and pretty much irrelevant (for the
kernel) compiler is a horrible thing to do.  And the only reason we do that
at all is that the compiler is from the same company that's behind ia64.
Please fix up your damn compiler.  Even SGI managed to add proper inline
asflembly support to Pro64, and they had much less ressources.
-
To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 5/5] fix sn rw_mmr.h to use intrinsic

[Luck, Tony]

On Tue, Jan 31, 2006 at 11:19:35AM +0000, Christoph Hellwig wrote:
> Agreed.  Obsfucating for a propritary and pretty much irrelevant (for the
> kernel) compiler is a horrible thing to do.  And the only reason we do that
> at all is that the compiler is from the same company that's behind ia64.
> Please fix up your damn compiler.  Even SGI managed to add proper inline
> as?embly support to Pro64, and they had much less ressources.

How about we un-obfuscate the code by moving these three functions to
a ".S" file?  They are reading from uncached physical memory, and are
packed full of "srlz.i" and ".acq"/".rel" options, so it doesn't appear
that moving them from inline to a full procedure call would make any
measurable difference to performance (at least not for any macrobenchmark).

pio_phys_read_mmr() isn't even used anyplace, so we could further clarify
the code by dropping it altogether.

-Tony

Re: [patch 5/5] fix sn rw_mmr.h to use intrinsic

[Jes Sorensen]

>>>>> "Tony" = Luck, Tony <tony.luck@intel.com> writes:

Tony> On Tue, Jan 31, 2006 at 11:19:35AM +0000, Christoph Hellwig
Tony> wrote:
>> Agreed.  Obsfucating for a propritary and pretty much irrelevant
>> (for the kernel) compiler is a horrible thing to do.  And the only
>> reason we do that at all is that the compiler is from the same
>> company that's behind ia64.  Please fix up your damn compiler.
>> Even SGI managed to add proper inline as?embly support to Pro64,
>> and they had much less ressources.

Tony> How about we un-obfuscate the code by moving these three
Tony> functions to a ".S" file?  They are reading from uncached
Tony> physical memory, and are packed full of "srlz.i" and
Tony> ".acq"/".rel" options, so it doesn't appear that moving them
Tony> from inline to a full procedure call would make any measurable
Tony> difference to performance (at least not for any macrobenchmark).

Tony,

We could do this in this particular case, but it doesn't solve the
fundamental problem. What we really need is Intel showing that it will
fix it's broken compilers once and for all if Intel wishes to continue
compiling the kernel with it.

The way it is right now, it means users who make the mistake of
compiling with ICC ends up exercising different codepaths than what
everybody else runs. This makes bug reports and benchmark results
meaningless.

What I don't understand is why this is so much more difficult for
Intel's compiler team to fix when several other, and much smaller,
teams haven't found it a problem to resolve it in the past.

Tony> pio_phys_read_mmr() isn't even used anyplace, so we could
Tony> further clarify the code by dropping it altogether.

I don't know if there's code in the pipeline which expects to use
this function. Anyone knows?

Again, this is my personal oppinion on this subject, it may differ
from that of my employer.

Regards,
Jes

Re: [patch 5/5] fix sn rw_mmr.h to use intrinsic

[Jack Steiner]

On Thu, Feb 02, 2006 at 04:39:23AM -0500, Jes Sorensen wrote:
> >>>>> "Tony" = Luck, Tony <tony.luck@intel.com> writes:
> 
> Tony> On Tue, Jan 31, 2006 at 11:19:35AM +0000, Christoph Hellwig
> Tony> wrote:
> >> Agreed.  Obsfucating for a propritary and pretty much irrelevant
> >> (for the kernel) compiler is a horrible thing to do.  And the only
> >> reason we do that at all is that the compiler is from the same
> >> company that's behind ia64.  Please fix up your damn compiler.
> >> Even SGI managed to add proper inline as?embly support to Pro64,
> >> and they had much less ressources.
> 
> Tony> How about we un-obfuscate the code by moving these three
> Tony> functions to a ".S" file?  They are reading from uncached
> Tony> physical memory, and are packed full of "srlz.i" and
> Tony> ".acq"/".rel" options, so it doesn't appear that moving them
> Tony> from inline to a full procedure call would make any measurable
> Tony> difference to performance (at least not for any macrobenchmark).

Sounds like a reasonable idea. I'll post the patch within a week. Is that
quick enough?


> 
> Tony,
> 
> We could do this in this particular case, but it doesn't solve the
> fundamental problem. What we really need is Intel showing that it will
> fix it's broken compilers once and for all if Intel wishes to continue
> compiling the kernel with it.
> 
> The way it is right now, it means users who make the mistake of
> compiling with ICC ends up exercising different codepaths than what
> everybody else runs. This makes bug reports and benchmark results
> meaningless.
> 
> What I don't understand is why this is so much more difficult for
> Intel's compiler team to fix when several other, and much smaller,
> teams haven't found it a problem to resolve it in the past.
> 
> Tony> pio_phys_read_mmr() isn't even used anyplace, so we could
> Tony> further clarify the code by dropping it altogether.
> 
> I don't know if there's code in the pipeline which expects to use
> this function. Anyone knows?

Nothing in the pipeline. I don't know of any longterm plans to use the 
function.  If we find a reason, the function is trivial to reinvent & 
add to the assembly function that contains the other function.


-- 
Thanks

Jack Steiner (steiner@sgi.com)          651-683-5302
Principal Engineer                      SGI - Silicon Graphics, Inc.

RE: [patch 5/5] fix sn rw_mmr.h to use intrinsic

[Luck, Tony]

> We could do this in this particular case, but it doesn't solve the
> fundamental problem. What we really need is Intel showing that it will
> fix it's broken compilers once and for all if Intel wishes to continue
> compiling the kernel with it.

This one is your call ... you are the official maintainer of this
file now[1].  I think that moving these to a ".S" file represents a
real cleanup (that just happens to help compiling with icc).

> The way it is right now, it means users who make the mistake of
> compiling with ICC ends up exercising different codepaths than what
> everybody else runs. This makes bug reports and benchmark results
> meaningless.

I think that there are some real benefits to building with a different
compiler.  icc has exposed some real bugs in the kernel in the past,
and I expect that it will do so again.

> What I don't understand is why this is so much more difficult for
> Intel's compiler team to fix when several other, and much smaller,
> teams haven't found it a problem to resolve it in the past.

Me too.  On the x86 side icc accepts inline asm(), and there have
been substantial improvements to icc to make it accept many of the
commonly used gcc extensions ... but the ia64 team have steadfastly
stuck to their position that intrinsics are the wave of the future.
As far as the kernel goes this has really slowed down acceptance (and
even use) of icc.  Kernel code contains few constructs where icc can
really shine over gcc, and in any case the C code is often written
with an eye on the instructions that are generated, which further
limits the possible upside.

> Again, this is my personal oppinion on this subject, it may differ
> from that of my employer.

I suspect that some of what I've written above may diverge
from my employer's views too.

-Tony

[1] http://tinyurl.com/7cqz8

Re: [patch 5/5] fix sn rw_mmr.h to use intrinsic

[Jes Sorensen]

>>>>> "Tony" = Luck, Tony <tony.luck@intel.com> writes:

>> We could do this in this particular case, but it doesn't solve the
>> fundamental problem. What we really need is Intel showing that it
>> will fix it's broken compilers once and for all if Intel wishes to
>> continue compiling the kernel with it.

Tony> This one is your call ... you are the official maintainer of
Tony> this file now[1].  I think that moving these to a ".S" file
Tony> represents a real cleanup (that just happens to help compiling
Tony> with icc).

Hi Tony,

Still haven't received the official 'Altix Sucker' t-shirt ;-) But I
agree it looks like a real cleanup that way. I noticed Jack
volunteered already, but I can look into it if Jack has too much other
stuff on his plate.

>> The way it is right now, it means users who make the mistake of
>> compiling with ICC ends up exercising different codepaths than what
>> everybody else runs. This makes bug reports and benchmark results
>> meaningless.

Tony> I think that there are some real benefits to building with a
Tony> different compiler.  icc has exposed some real bugs in the
Tony> kernel in the past, and I expect that it will do so again.

I don't think it's an invalid thing to do, if nothing else it's a
great point for the compiler people as a stress test. It just ought to
be done on the kernel's terms not the other way round.

>> What I don't understand is why this is so much more difficult for
>> Intel's compiler team to fix when several other, and much smaller,
>> teams haven't found it a problem to resolve it in the past.

Tony> Me too.  On the x86 side icc accepts inline asm(), and there
Tony> have been substantial improvements to icc to make it accept many
Tony> of the commonly used gcc extensions ... but the ia64 team have
Tony> steadfastly stuck to their position that intrinsics are the wave
Tony> of the future.  As far as the kernel goes this has really slowed
Tony> down acceptance (and even use) of icc.  Kernel code contains few
Tony> constructs where icc can really shine over gcc, and in any case
Tony> the C code is often written with an eye on the instructions that
Tony> are generated, which further limits the possible upside.

Sounds like we're on the same page! Next question is what it would
take to make them see the light? Pulling out the intrinsics code would
be one way, but it might cause a lot of screaming on the short term
;-)

Cheers,
Jes

Re: [patch 5/5] fix sn rw_mmr.h to use intrinsic

[Gerald Pfeifer]

On Thu, 2 Feb 2006, Jes Sorensen wrote:
> We could do this in this particular case, but it doesn't solve the
> fundamental problem. What we really need is Intel showing that it will
> fix it's broken compilers once and for all if Intel wishes to continue
> compiling the kernel with it.

One thing I've always been wondering about: are there any relevant
users running ICC compiled kernels for real, or is this just an academic 
exercise?

Those distros I know about, and in at least one case I can assert this 
quite authoritatively ;-), never will go down this path, so I assume
there must be some adventurous users running home grown kernels?

Gerald