GPU string overhaul #1531
GPU string overhaul #1531
Conversation
Big string cleanup, yet again, for GPU. We still had this tricky double indirection happening with strings for OptiX -- the string was a pointer to a "global", which in turn held the hash. No, throw all that out. Just directly store strings as the ustring hash on OptiX. No extra indirection. Many fewer differences between how things are handled on OptiX versus CPU (except for the fact itself that the "string" holds the hash rather than the char*). Some details worth noting: * LLVM_Util loses "type_string()" and calls it "type_ustring()" to emphasize it's the type we're using to represent a ustring, and definitely not a C or C++std string. Also, LLVM_Util itself now lets the caller tell it whether it prefers that the representation of a ustring be the charptr or the hash, and then handles most of the details internally. The BackendLLVM, then, makes that decision early on, charptr for CPU and hash for GPU. * Several more spots in LLVM_Util and BackendLLVM have been plumbed to pass around llvm IR SSA variable names to try to make the resulting LLVM IR more readable when I'm trying to debug it. (This was strictly debugging scaffalding aid for me, but comes along for the ride because it's useful to keep.) I also added a BackendLLVM::llnamefmt to format strings, which only does the formatting when the shading system knows it's going to save the IR for inspection, and otherwise (that is, in production) it does not do the string formatting or pass the names along, so it's not doing all that string manipulation when no human is going to be looking at the IR dumps. * A lot of clunky OptiX-only code disappears. * Some side effects of my reducing complexity of the IR as I debugged: there were times we were generating redundant pointer casting in the IR, such as grabbing a pointer, then casting it to a void*, then casting back to some other kind of pointer. In some cases I reduced that quite a bit. Again, this was mostly just a scramble to make the IR more readable while I debugged, but it probably has a minor performance gain as well since it reduced somewhat the sheer amount of IR we generate and pass to the optimizer and JIT. Signed-off-by: Larry Gritz <lg@larrygritz.com>
|
@timgrant Would be great to get your eyeballs on this, too. |
|
Taking a look now. |
src/include/OSL/rendererservices.h
Outdated
| @@ -411,7 +411,8 @@ class OSLEXECPUBLIC RendererServices { | |||
| virtual uint64_t register_string (const std::string& str, | |||
There was a problem hiding this comment.
It looks like register_string is vestigial now. Does it make sense to remove it? Or maybe there is some potential use beyond a fancy wrapper for ustring::hash?
There was a problem hiding this comment.
I would be in favor of removing it if not used.
There was a problem hiding this comment.
I think you're right, but I wasn't sure about the compatibility break that would be incurred to removing a virtual method from the parent class. Maybe I'll just mark it as deprecated first?
There was a problem hiding this comment.
This is only for the next release, so I don't think this is too critical.
There was a problem hiding this comment.
YOLO, removed it entirely.
There was a problem hiding this comment.
I'm sure you would have noticed, but if not it looks like there are a couple lingering uses of the function in the test programs that need a little bit of refactoring.
There was a problem hiding this comment.
Oops, yeah, fixing.
There was a problem hiding this comment.
Updated. Builds fine now.
There was a problem hiding this comment.
Other than this, did the overall simplification look sane to you, @tgrant-nv ?
There was a problem hiding this comment.
Yes, definitely. It's a huge improvement in readability and maintainability for GPU strings. I don't think anyone will miss trying to reason about load_device_string.
|
That'd be my vote. I think we'd be ok with it removed, but I'd rather wait
for the dust to settle a little bit once the new functionality is pushed
out to make sure all the pieces are in place on our side to make sure we
won't need it.
…On Mon, Jul 11, 2022 at 10:41 AM Larry Gritz ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/include/OSL/rendererservices.h
<#1531 (comment)>
:
> @@ -411,7 +411,8 @@ class OSLEXECPUBLIC RendererServices {
virtual uint64_t register_string (const std::string& str,
I think you're right, but I wasn't sure about the compatibility break that
would be incurred to removing a virtual method from the parent class. Maybe
I'll just mark it as deprecated first?
—
Reply to this email directly, view it on GitHub
<#1531 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEULLERPWR7SJU2FAPANO3VTRMDDANCNFSM526KREQQ>
.
You are receiving this because your review was requested.Message ID:
<AcademySoftwareFoundation/OpenShadingLanguage/pull/1531/review/1034712910
@github.com>
|
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
|
Will merge this at the end of the day. Register any final objections now. |
Big string cleanup, yet again, for GPU. We still had this tricky
double indirection happening with strings for OptiX -- the string was
a pointer to a "global", which in turn held the hash. No, throw all
that out. Just directly store strings as the ustring hash on
OptiX. No extra indirection. Many fewer differences between how things
are handled on OptiX versus CPU (except for the fact itself that the
"string" holds the hash rather than the char*).
Some details worth noting:
LLVM_Util loses "type_string()" and calls it "type_ustring()" to
emphasize it's the type we're using to represent a ustring, and
definitely not a C or C++std string. Also, LLVM_Util itself now lets
the caller tell it whether it prefers that the representation of a
ustring be the charptr or the hash, and then handles most of the
details internally. The BackendLLVM, then, makes that decision early
on, charptr for CPU and hash for GPU.
Several more spots in LLVM_Util and BackendLLVM have been plumbed to
pass around llvm IR SSA variable names to try to make the resulting
LLVM IR more readable when I'm trying to debug it. (This was
strictly debugging scaffalding aid for me, but comes along for the
ride because it's useful to keep.) I also added a
BackendLLVM::llnamefmt to format strings, which only does the
formatting when the shading system knows it's going to save the IR
for inspection, and otherwise (that is, in production) it does not
do the string formatting or pass the names along, so it's not doing
all that string manipulation when no human is going to be looking at
the IR dumps.
A lot of clunky OptiX-only code disappears.
Some side effects of my reducing complexity of the IR as I debugged:
there were times we were generating redundant pointer casting in the
IR, such as grabbing a pointer, then casting it to a void*, then
casting back to some other kind of pointer. In some cases I reduced
that quite a bit. Again, this was mostly just a scramble to make the
IR more readable while I debugged, but it probably has a minor
performance gain as well since it reduced somewhat the sheer amount
of IR we generate and pass to the optimizer and JIT.
Signed-off-by: Larry Gritz lg@larrygritz.com