fix: Handle mixed string casting that plagues optix codegen #1718
Conversation
The string situation is still a bit tricky on the OptiX side. There's a full overhaul of that coming separately as we continue to shift to an all-ustringhash world. But in the mean time, we were getting some testsuite failures that were because of a mismatch between times we think of a gpu-side ustring as a char* and when it's a uint64. IN FACT, they are always the hashes, but we backed ourselves into a corner where sometimes that 64 bit pattern is passed around as a pointer, ugh. This small patch attempts to catch this case and just do the appropriate cast to compensate. It's a band-aid, a more comprehensive fix is coming, as I said. This gets us down to just ONE filing optix test case, which I will tackle separately. Signed-off-by: Larry Gritz <[email protected]>
src/liboslexec/llvm_util.cpp
Outdated
| @@ -5216,6 +5224,7 @@ LLVM_Util::op_store(llvm::Value* val, llvm::Value* ptr) | |||
| std::cerr << "op_store val->getType()=" << std::flush; | |||
| val->getType()->print(llvm::errs()); | |||
| std::cerr << std::endl; | |||
| // OSL_ASSERT(0); | |||
There was a problem hiding this comment.
Remove commented out line(?)
There was a problem hiding this comment.
I want to leave that for now to make it easy to add an assertion at that spot if it's triggered again.
There was a problem hiding this comment.
Wait, maybe I'll change it to DASSERT.
There was a problem hiding this comment.
I think my point was that it was currently commented out, so doing nothing.
Having it OSL_ASSERT or OSL_ASSERT is useful when in debugger because you get the builtin_trap and break right there.
There was a problem hiding this comment.
Commented out DOES SOMETHING -- it reminds me exactly where to put the assertion next time I have to track down another instance of the problem, and a single keystroke will activate it, but in the mean time it won't produce any extra error messages or stop a running program.
There was a problem hiding this comment.
Updated to not-commented-out OSL_DASSERT, so it will hit the assertion in debug mode, but not for production builds.
Signed-off-by: Larry Gritz <[email protected]>
lgritz
merged commit a4e779c
into
AcademySoftwareFoundation:main
* build(deps): Raise minimum CMake dependency from 3.12 to 3.15 (AcademySoftwareFoundation#1724) * Change RendererServices::get_texture_handle back to ustring (AcademySoftwareFoundation#1726) * The recently added SS::find_symloc can have const args (AcademySoftwareFoundation#1723) * perf(gpu): Move qualifying GroupData params onto stack (AcademySoftwareFoundation#1710) * OptiX PTX pipeline overhaul (AcademySoftwareFoundation#1680) * Fix default fmt logic changing in latest openimageio release (AcademySoftwareFoundation#1725) * feat: Add API for building attribute getter free functions. (AcademySoftwareFoundation#1704) * fix: Handle mixed string casting that plagues optix codegen (AcademySoftwareFoundation#1718) * Rs fmt specification - Journaling Algorithm for error, warning, fprintf (AcademySoftwareFoundation#1702)
The string situation is still a bit tricky on the OptiX side. There's a full overhaul of that coming separately as we continue to shift to an all-ustringhash world.
But in the mean time, we were getting some testsuite failures that were because of a mismatch between times we think of a gpu-side ustring as a char* and when it's a uint64. IN FACT, they are always the hashes, but we backed ourselves into a corner where sometimes that 64 bit pattern is passed around as a pointer, ugh.
This small patch attempts to catch this case and just do the appropriate cast to compensate. It's a band-aid; a more comprehensive fix is coming, as I said.
This gets us down to just ONE failing optix test case, which I will tackle separately.
The change necessitated adding a new
ptr_to_int64_cast()method to LLVM_Util.