Date   

Re: check_nan bug fix and skip bind for struct params (issue204077)

Larry Gritz <l...@...>
 

We should double-check that the placeholder struct symbol table entries don't take any space in the heap. They aren't intended to.

If you don't have time to do it, please just file a ticket and I'll take a look later.

-- lg


On Feb 8, 2010, at 1:17 PM, <cku...@...> <cku...@...> wrote:

I would have to check again, it seems to me the size was > 0. In any
case, it seemed dangerous to give a valid heap address to symbols that
should never be used.

The nan fix is just that we were checking up to <= endpoint instead of <
endpoint.

http://codereview.appspot.com/204077/show
--
Larry Gritz
l...@...


sincos opcode (issue205045)

cku...@...
 

Reviewers: dev-osl_imageworks.com, osl-dev_googlegroups.com,

Description:
Add a new opcode that computes sin and cos together

Please review this at http://codereview.appspot.com/205045/show

Affected files:
src/liboslcomp/typecheck.cpp
src/liboslexec/master.cpp
src/liboslexec/opmathfunc.cpp
src/liboslexec/oslops.h
testsuite/trig/ref/out.txt
testsuite/trig/test.osl


Re: sincos opcode (issue205045)

larry...@...
 

Dude, use the C++ sincos() function to compute the sines and cosines!
You may as well make the underlying implementation faster in the same
way.

Other than that, LGTM.


http://codereview.appspot.com/205045/show


Re: sincos opcode (issue205045)

cku...@...
 

On 2010/02/08 23:18:07, larrygritz wrote:
Dude, use the C++ sincos() function to compute the sines and cosines!
You may
as well make the underlying implementation faster in the same way.
Other than that, LGTM.
This is a GNU extension, it isn't part of any standard to my knowledge.
Can you think of a cross platform way of detecting it?

http://codereview.appspot.com/205045/show


Re: sincos opcode (issue205045)

Larry Gritz <l...@...>
 

I would put in fmath.h:

#ifndef __GNUC__
... define it in terms of sin and cos...
#endif



On Feb 8, 2010, at 3:22 PM, <cku...@...> wrote:

On 2010/02/08 23:18:07, larrygritz wrote:
Dude, use the C++ sincos() function to compute the sines and cosines!
You may
as well make the underlying implementation faster in the same way.
Other than that, LGTM.
This is a GNU extension, it isn't part of any standard to my knowledge.
Can you think of a cross platform way of detecting it?

http://codereview.appspot.com/205045/show
--
Larry Gritz
l...@...


Re: sincos opcode (issue205045)

cku...@...
 

Its more complicated than that. The symbol doesn't show up by default.
You need to do:

#define _GNU_SOURCE
#include <math.h>

And of course, its hard to know where math.h (or cmath) is first
included (this can change depending on the order of inclusion of the
headers).

This only works on linux by the way, os x does not support this
extension as far as I can tell.


http://codereview.appspot.com/205045/show


Re: sincos opcode (issue205045)

Larry Gritz <l...@...>
 

Of course, please substitute whatever compiler or version checks *correctly* discern whether sincos is available.


On Feb 8, 2010, at 3:25 PM, Larry Gritz wrote:

I would put in fmath.h:

#ifndef __GNUC__
... define it in terms of sin and cos...
#endif



On Feb 8, 2010, at 3:22 PM, <cku...@...> wrote:

On 2010/02/08 23:18:07, larrygritz wrote:
Dude, use the C++ sincos() function to compute the sines and cosines!
You may
as well make the underlying implementation faster in the same way.
Other than that, LGTM.
This is a GNU extension, it isn't part of any standard to my knowledge.
Can you think of a cross platform way of detecting it?

http://codereview.appspot.com/205045/show
--
Larry Gritz
l...@...


--
Larry Gritz
l...@...


Re: sincos opcode (issue205045)

Larry Gritz <l...@...>
 

1. At the top of fmath.h, before #include <cmath>:

#ifndef __MATH_H__
#define _GNU_SOURCE
#endif


2. In fmath.h, define sincos

#if !defined(_GNU_SOURCE) || !defined(__GNUC__) || !defined(__linux__)
inline void sincos (...) {...}
#endif

3. Tough luck for users who #include <math.h> BEFORE fmath.h and don't define _GNU_SOURCE, they'll get the backup definition, that calls sin and cos separately.

This will work for everybody and never be slower than using sin and cos separately. But users who care enough to do the right #defines and #includes will get something faster.


On Feb 8, 2010, at 3:31 PM, <cku...@...> wrote:

Its more complicated than that. The symbol doesn't show up by default.
You need to do:

#define _GNU_SOURCE
#include <math.h>

And of course, its hard to know where math.h (or cmath) is first
included (this can change depending on the order of inclusion of the
headers).

This only works on linux by the way, os x does not support this
extension as far as I can tell.


http://codereview.appspot.com/205045/show

--
You received this message because you are subscribed to the Google Groups "OSL Developers" group.
To post to this group, send email to osl...@....
To unsubscribe from this group, send email to osl...@....
For more options, visit this group at http://groups.google.com/group/osl-dev?hl=en.

--
Larry Gritz
l...@...


Re: sincos opcode (issue205045)

cku...@...
 

I found an alternative that gets around having to use _GNU_SOURCE (which
might have other weird side effects).

http://codereview.appspot.com/205045/show


Re: sincos opcode (issue205045)

cku...@...
 

using portable sincos instead of calls to sin and cos

http://codereview.appspot.com/205045/show


Re: sincos opcode (issue205045)

cku...@...
 

On 2010/02/09 01:27:22, ckulla wrote:
using portable sincos instead of calls to sin and cos
Here is the patch to fmath.h:

Index: src/include/fmath.h
===================================================================
--- src/include/fmath.h (revision 1392)
+++ src/include/fmath.h (working copy)
@@ -779,7 +779,29 @@
}


+inline void
+sincos(float x, float* sine, float* cosine)
+{
+#if defined(__GNUC__) && defined(__linux__)
+ __builtin_sincosf(x, sin, cosine);
+#else
+ *sine = std::sin(x);
+ *cosine = std::cos(x);
+#endif
+}

+inline void
+sincos(double x, double* sine, double* cosine)
+{
+#if defined(__GNUC__) && defined(__linux__)
+ __builtin_sincos(x, sin, cosine);
+#else
+ *sine = std::sin(x);
+ *cosine = std::cos(x);
+#endif
+}
+
+
#ifdef OPENIMAGEIO_NAMESPACE
}; // end namespace OPENIMAGEIO_NAMESPACE
using namespace OPENIMAGEIO_NAMESPACE;


http://codereview.appspot.com/205045/show


Re: sincos opcode (issue205045)

Blair Zajac <bl...@...>
 

Solaris provides sincos as part of math.h.

Given that OSL is now an open-source project, probably at some point it'll need to grow a configure based solution to find functions.

If configure was used, then it would build a config.h that would define or not define HAVE_SINCOS and then the code could just rely upon that, instead of trusting a developer to include headers in the correct order or checking if it was a particular platform.

Regards,
Blair

On 02/08/2010 03:48 PM, Larry Gritz wrote:
1. At the top of fmath.h, before #include<cmath>:

#ifndef __MATH_H__
#define _GNU_SOURCE
#endif


2. In fmath.h, define sincos

#if !defined(_GNU_SOURCE) || !defined(__GNUC__) || !defined(__linux__)
inline void sincos (...) {...}
#endif

3. Tough luck for users who #include<math.h> BEFORE fmath.h and don't define _GNU_SOURCE, they'll get the backup definition, that calls sin and cos separately.

This will work for everybody and never be slower than using sin and cos separately. But users who care enough to do the right #defines and #includes will get something faster.


On Feb 8, 2010, at 3:31 PM,<cku...@...> wrote:

Its more complicated than that. The symbol doesn't show up by default.
You need to do:

#define _GNU_SOURCE
#include<math.h>

And of course, its hard to know where math.h (or cmath) is first
included (this can change depending on the order of inclusion of the
headers).

This only works on linux by the way, os x does not support this
extension as far as I can tell.


http://codereview.appspot.com/205045/show


Pre-review: run state overhaul (issue206045)

larry...@...
 

Reviewers: ,

Description:
We've done extensive analysis showing that for typical shading batches
we see in the wild, our method of having an array of true/false runflags
is very wasteful, and despite the extra indirection involved, having a
flat list of the "on" points is much more efficient (5x or more speed
gain on tight inner loops). We're not sure yet exactly what this will
translate to in overall runtime gains, but it's got to help.

This is not a final review, just showing progress along the way.

The code in this pre-review adds the index list to the Runstate, and
maintains it properly. I validate this by checking at every instruction
that the runflags and indices, and all tests pass! So I know for sure
that the indices are being set up properly.

The whole point is that now I can convert the shadeops one by one, at
all times keeping the renderer working, and then only when everything is
converted rip out the old runflags entirely.


Please review this at http://codereview.appspot.com/206045/show

Affected files:
src/liboslexec/exec.cpp
src/liboslexec/opcontrol.cpp
src/liboslexec/oslexec_pvt.h


Re: Pre-review: run state overhaul (issue206045)

larry...@...
 

Incidentally, how much slower do we render by keeping both the runflags
and indices in place? Only 2%.

So this makes me think that quite soon into changing the individual
shadeops, we'll break even and can start deploying a half-completed
solution that's a performance improvement, even long before we've
finished the whole thing and are ready to rip out the runflags once and
for all.



http://codereview.appspot.com/206045/show


Re: Pre-review: run state overhaul (issue206045)

aco...@...
 


Re: Pre-review: run state overhaul (issue206045)

cku...@...
 

LGTM2


http://codereview.appspot.com/206045/diff/1/4
File src/liboslexec/exec.cpp (right):

http://codereview.appspot.com/206045/diff/1/4#newcode583
src/liboslexec/exec.cpp:583: #if 1
Should this be #if 0'd ? Or is it really only 2% slower with this in
place?

http://codereview.appspot.com/206045/show


Re: Pre-review: run state overhaul (issue206045)

Larry Gritz <l...@...>
 

No, it's 2% slower with it '#if 0'.

I'm not going to commit in this state, I just wanted to show everybody how I'd verified that I'm maintaining the index list correctly and the general scheme of what we're doing.

-- lg

On Feb 9, 2010, at 10:16 AM, <cku...@...> wrote:

LGTM2


http://codereview.appspot.com/206045/diff/1/4
File src/liboslexec/exec.cpp (right):

http://codereview.appspot.com/206045/diff/1/4#newcode583
src/liboslexec/exec.cpp:583: #if 1
Should this be #if 0'd ? Or is it really only 2% slower with this in
place?

http://codereview.appspot.com/206045/show
--
Larry Gritz
l...@...


Re: sincos opcode (issue205045)

cku...@...
 

gcc documentation suggests that it can (macro TARGET_HAS_SINCOS) - but
our version doesn't seem to do it automatically.

In theory its true that it would be nice to just let the compiler take
care of this.

http://codereview.appspot.com/205045/show


Re: array assign can nuke derivatives (issue205043)

cku...@...
 

On 2010/02/08 19:54:50, ckulla wrote:


Any comments on this?

http://codereview.appspot.com/205043/show


Re: array assign can nuke derivatives (issue205043)

cliffo...@...
 

On 2010/02/09 21:40:51, ckulla wrote:
On 2010/02/08 19:54:50, ckulla wrote:
Any comments on this?
LGTM

http://codereview.appspot.com/205043/show

201 - 220 of 5013