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

Description:
We were calling noise functions osl_psnoise_dfdfdfff with the wrong type
for the last two arguments. That was triggering an assert from LLVM.
This patch correctly produces floats for those two arguments whose
derivatives are ignored.

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

Affected files:
src/liboslexec/llvm_instance.cpp


Index: src/liboslexec/llvm_instance.cpp
===================================================================
--- src/liboslexec/llvm_instance.cpp (revision 798)
+++ src/liboslexec/llvm_instance.cpp (working copy)
@@ -2405,42 +2405,47 @@ LLVMGEN (llvm_gen_pnoise)
int firstperiod = (op.nargs() - 1) / 2 + 1;

Symbol& Result = *rop.opargsym (op, 0);
- std::vector<const Symbol *> args;
bool any_deriv_args = false;
for (int i = 0; i < op.nargs(); ++i) {
Symbol *s (rop.opargsym (op, i));
- args.push_back (s);
any_deriv_args |= (i > 0 && i < firstperiod &&
s->has_derivs() && !s->typespec().is_matrix());
}

std::string name = std::string("osl_") + op.opname().string() + "_";
+ std::vector<llvm::Value *> valargs;
+ valargs.resize (op.nargs());
for (int i = 0; i < op.nargs(); ++i) {
Symbol *s (rop.opargsym (op, i));
- if (any_deriv_args && i < firstperiod && Result.has_derivs() &&
- s->has_derivs() && !s->typespec().is_matrix())
+ bool use_derivs = any_deriv_args && i < firstperiod && Result.has_derivs() && s->has_derivs() && !s->typespec().is_matrix();
+ if (use_derivs)
name += "d";
if (s->typespec().is_float())
name += "f";
else if (s->typespec().is_triple())
name += "v";
else ASSERT (0);
+
+
+ if (s->typespec().simpletype().aggregate > 1 || use_derivs)
+ valargs[i] = rop.llvm_void_ptr (*s);
+ else
+ valargs[i] = rop.llvm_load_value (*s);
}

if (! Result.has_derivs() || ! any_deriv_args) {
// Don't compute derivs -- either not needed or not provided in args
if (Result.typespec().aggregate() == TypeDesc::SCALAR) {
- llvm::Value *r = rop.llvm_call_function (name.c_str(),
- &(args[1]), op.nargs()-1);
+ llvm::Value *r = rop.llvm_call_function (name.c_str(), &valargs[1], op.nargs()-1);
rop.llvm_store_value (r, Result);
} else {
- rop.llvm_call_function (name.c_str(), &(args[0]), op.nargs());
+ rop.llvm_call_function (name.c_str(), &valargs[0], op.nargs());
}
rop.llvm_zero_derivs (Result);
} else {
// Cases with derivs
ASSERT (Result.has_derivs() && any_deriv_args);
- rop.llvm_call_function (name.c_str(), &(args[0]), op.nargs(), true);
+ rop.llvm_call_function (name.c_str(), &valargs[0], op.nargs());
}
return true;
}

http://codereview.appspot.com/1976041/diff/1/2
File src/liboslexec/llvm_instance.cpp (right):

http://codereview.appspot.com/1976041/diff/1/2#newcode2420
src/liboslexec/llvm_instance.cpp:2420: bool use_derivs = any_deriv_args
&& i < firstperiod && Result.has_derivs() && s->has_derivs() &&
!s->typespec().is_matrix();
Why do we check for !is_matrix() ?

http://codereview.appspot.com/1976041/diff/1/2#newcode2430
src/liboslexec/llvm_instance.cpp:2430: if
(s->typespec().simpletype().aggregate > 1 || use_derivs)
Couldn't this logic be inside of llvm_load_value?

Aren't floats/ints always passed by value, while other types are always
passed by void* ?

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

any_deriv_args && i <

firstperiod && Result.has_derivs() && s->has_derivs() &&
!s->typespec().is_matrix();
Why do we check for !is_matrix() ?

You'll have to ask Larry, I just tried to change the code as less as
possible to make it work.

http://codereview.appspot.com/1976041/diff/1/2#newcode2430
src/liboslexec/llvm_instance.cpp:2430: if

(s->typespec().simpletype().aggregate

1 || use_derivs)

Couldn't this logic be inside of llvm_load_value?

Well, I didn't want to get into that for this review.

Aren't floats/ints always passed by value, while other types are

always passed

by void* ?

Not sure about this, but it seems that when a symbol has derivatives you
can either grab the value or the pointer to the Dual.

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

LGTM


http://codereview.appspot.com/1976041/diff/1/2
File src/liboslexec/llvm_instance.cpp (right):

http://codereview.appspot.com/1976041/diff/1/2#newcode2420
src/liboslexec/llvm_instance.cpp:2420: bool use_derivs = any_deriv_args
&& i < firstperiod && Result.has_derivs() && s->has_derivs() &&
!s->typespec().is_matrix();
On 2010/08/11 21:50:05, ckulla wrote:

Why do we check for !is_matrix() ?

I think we don't currently support matrix derivatives anywhere.

http://codereview.appspot.com/1976041/diff/1/2#newcode2430
src/liboslexec/llvm_instance.cpp:2430: if
(s->typespec().simpletype().aggregate > 1 || use_derivs)
On 2010/08/11 21:50:05, ckulla wrote:

Couldn't this logic be inside of llvm_load_value?

No, because sometimes you want to pass by value, other times you need a
pointer (like if it's an output arg, or needs the derivs accessable).
llvm_load_value isn't smart enough to know how it's going to be used.

Aren't floats/ints always passed by value, while other types are

always passed

by void* ?

Yes. But also pass by reference (i.e. void*) if it's an output var, or
needs derivs.

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

Checked in to r799

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

http://codereview.appspot.com/1976041/diff/1/2
File src/liboslexec/llvm_instance.cpp (right):

http://codereview.appspot.com/1976041/diff/1/2#newcode2420
src/liboslexec/llvm_instance.cpp:2420: bool use_derivs = any_deriv_args
&& i < firstperiod && Result.has_derivs() && s->has_derivs() &&
!s->typespec().is_matrix();
On 2010/08/11 23:16:33, larrygritz wrote:

On 2010/08/11 21:50:05, ckulla wrote:

Why do we check for !is_matrix() ?

I think we don't currently support matrix derivatives anywhere.

So wouldn't this be caught by s->has_derivs() ? And which noise function
call takes a matrix as an argument?



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