Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1399)

Issue 1976041: Fix arg types mismatch in noise call (LLVM) (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 10 months ago by aconty
Modified:
13 years, 4 months ago
Reviewers:
dev-osl <dev-osl, larrygritz, osl-dev, ckulla
Base URL:
http://openshadinglanguage.googlecode.com/svn/trunk/
Visibility:
Public.

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.

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -8 lines) Patch
src/liboslexec/llvm_instance.cpp View 1 chunk +13 lines, -8 lines 5 comments Download

Messages

Total messages: 6
aconty
13 years, 10 months ago (2010-08-11 21:38:33 UTC) #1
ckulla
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 && ...
13 years, 10 months ago (2010-08-11 21:50:05 UTC) #2
aconty
any_deriv_args && i < > firstperiod && Result.has_derivs() && s->has_derivs() && > !s->typespec().is_matrix(); > Why ...
13 years, 10 months ago (2010-08-11 22:00:04 UTC) #3
larrygritz
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 ...
13 years, 10 months ago (2010-08-11 23:16:33 UTC) #4
aconty
Checked in to r799
13 years, 10 months ago (2010-08-12 00:22:27 UTC) #5
ckulla
13 years, 10 months ago (2010-08-12 02:31:48 UTC) #6
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?

>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b