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

Description:
I forgot to cover the case of external closures in the merge code for
ADD. This patch fixes the problem.

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

Affected files:
src/include/oslclosure.h
src/liboslexec/closure.cpp


Index: src/include/oslclosure.h
===================================================================
--- src/include/oslclosure.h (revision 786)
+++ src/include/oslclosure.h (working copy)
@@ -563,6 +563,10 @@ public:
return (void *)&m_mem[component(i).memoffset];
}

+ int id (int i) const {
+ return component(i).id;
+ }
+
/// Return whether the component is a builtin closure
bool is_builtin (int i) const { return component(i).id < NBUILTIN_CLOSURES; }

@@ -578,9 +582,10 @@ private:
int id; ///< Id of the componente
Color3 weight; ///< Weight of this component
int memoffset; ///< Offset at which we can find a ClosurePrimitive*
+ int memsize; ///< Memory size of the component

- Component (int id, const Color3 &weight, int memoffset) :
- id(id), weight(weight), memoffset(memoffset) { }
+ Component (int id, const Color3 &weight, int memoffset, int memsize) :
+ id(id), weight(weight), memoffset(memoffset), memsize(memsize) { }
};

/// Return the i-th component of this closure.
Index: src/liboslexec/closure.cpp
===================================================================
--- src/liboslexec/closure.cpp (revision 786)
+++ src/liboslexec/closure.cpp (working copy)
@@ -67,7 +67,7 @@ ClosureColor::allocate_component (int id, size_t num_bytes)

// Make a new component
m_components.clear();
- m_components.push_back (Component (id, Color3 (1, 1, 1), 0));
+ m_components.push_back (Component (id, Color3 (1, 1, 1), 0, num_bytes));

// Return the block of memory for the caller to new the ClosurePrimitive into
return &m_mem[0];
@@ -85,27 +85,29 @@ ClosureColor::add (const ClosureColor &A)
size_t new_bytes = 0; // how much more mem I'll need
int *unmerged = ALLOCA (int, A.ncomponents()); // temp index list
for (int ac = 0; ac < A.ncomponents(); ++ac) {
- const ClosurePrimitive *aprim (A.prim (ac));
- const Component &acomp (A.component (ac));
- if (acomp.weight[0] == 0.0f && acomp.weight[1] == 0.0f &&
- acomp.weight[2] == 0.0f)
- continue; // don't bother adding a 0-weighted component
bool merged = false;
- for (int c = 0; c < my_ncomponents; ++c) {
- if (prim(c)->name() == aprim->name() &&
- prim(c)->mergeable (aprim)) {
- // We can merge with an existing component -- just add the
- // weights
- m_components[c].weight += acomp.weight;
- merged = true;
- break;
+ const Component &acomp (A.component (ac));
+ if (!A.is_builtin(ac)) {
+ const ClosurePrimitive *aprim (A.prim (ac));
+ if (acomp.weight[0] == 0.0f && acomp.weight[1] == 0.0f &&
+ acomp.weight[2] == 0.0f)
+ continue; // don't bother adding a 0-weighted component
+ for (int c = 0; c < my_ncomponents; ++c) {
+ if (is_builtin(c) && prim(c)->name() == aprim->name() &&
+ prim(c)->mergeable (aprim)) {
+ // We can merge with an existing component -- just add the
+ // weights
+ m_components[c].weight += acomp.weight;
+ merged = true;
+ break;
+ }
}
}
if (! merged) {
// Not a duplicate that can be merged. Remember this component
// index and how much memory it'll need.
unmerged[num_unmerged++] = ac;
- new_bytes += aprim->memsize();
+ new_bytes += acomp.memsize;
}
}

@@ -121,8 +123,7 @@ ClosureColor::add (const ClosureColor &A)
for (int i = 0; i < num_unmerged; ++i) {
int c = unmerged[i]; // next unmerged component index within A
const Component &acomp (A.component (c));
- const ClosurePrimitive *aprim (A.prim (c));
- size_t asize = aprim->memsize();
+ size_t asize = acomp.memsize;
memcpy (&m_mem[oldmemsize], &A.m_mem[acomp.memoffset], asize);
m_components.push_back (acomp);
m_components.back().memoffset = oldmemsize;

Added support for merging external closures too

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

support for llvm side which I forgot in the previous patch

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

http://codereview.appspot.com/1876042/diff/15001/16001
File src/include/oslclosure.h (right):

http://codereview.appspot.com/1876042/diff/15001/16001#newcode583
src/include/oslclosure.h:583: int memsize; ///< Memory size of the
component
This isn't really needed. Its only used in the merge, where it could be
looked up from the closure registry.

http://codereview.appspot.com/1876042/diff/15001/16003
File src/liboslexec/builtin_closures.cpp (right):

http://codereview.appspot.com/1876042/diff/15001/16003#newcode189
src/liboslexec/builtin_closures.cpp:189: ss->register_closure
(clinfo->name, cid, clinfo->params, size, clinfo->prepare,
generic_closure_setup, NULL,
Why not just have generic_closure_merge here? This would simplify the
code in add.

http://codereview.appspot.com/1876042/diff/15001/16004
File src/liboslexec/closure.cpp (right):

http://codereview.appspot.com/1876042/diff/15001/16004#newcode93
src/liboslexec/closure.cpp:93: if (A.is_builtin(ac)) {
You have duplicated code here, I think it would be simpler to just have
a function pointer take care of the generic case too.

http://codereview.appspot.com/1876042/diff/15001/16009
File src/liboslexec/oslexec_pvt.h (right):

http://codereview.appspot.com/1876042/diff/15001/16009#newcode494
src/liboslexec/oslexec_pvt.h:494: ASSERT((size_t)id <
m_closure_table.size());
DASSERT

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

Addressed comments

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

LGTM, just a few small notes.


http://codereview.appspot.com/1876042/diff/31001/32004
File src/liboslexec/closure.cpp (right):

http://codereview.appspot.com/1876042/diff/31001/32004#newcode95
src/liboslexec/closure.cpp:95: CompareClosureFunc compare = ss ?
closure->compare : NULL;
Is this null check really needed?

The only case it comes up is probably the closure_test program ... but
since it didn't crash on the line above, it probably isn't going through
this code path anyway.

http://codereview.appspot.com/1876042/diff/31001/32006
File src/liboslexec/llvm_instance.cpp (right):

http://codereview.appspot.com/1876042/diff/31001/32006#newcode1667
src/liboslexec/llvm_instance.cpp:1667: llvm::errs() << "val is " << *val
<< " but Result is a " << Result.typespec().c_str() << ' ' <<
Result.mangled() << "\n";

Is this for debugging another issue?

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

Got it, I'll fix all these problems.

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

Addressed comments and debug clean up

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

On 2010/08/02 16:31:29, aconty wrote:

Addressed comments and debug clean up

LGTM

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

checked in to r791

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