-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pr/batched point cloud #1464
Merged
lgritz
merged 13 commits into
AcademySoftwareFoundation:main
from
AlexMWells:PR/BatchedPointCloud
Feb 19, 2022
Merged
Pr/batched point cloud #1464
lgritz
merged 13 commits into
AcademySoftwareFoundation:main
from
AlexMWells:PR/BatchedPointCloud
Feb 19, 2022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rite. Updated behavior of scalar pointcloud_search and pointcloud_get to not code gen branch with code gen runtime error for maxpoints/num_points larger than array size. Instead now it codgens a clamp of the maxpoints/num_points to the arraysize. This allows code to continue executing, and if accessed outside of the range, existing codgen range checks will still emit a runtime error. Extracted class PointCloud defnition and some helper functions to pointcloud.h so it can be shared between batched and scalar implementation. Moved PointCloud out of anonymous namespace to OSL::pvt so a single implementation (including static maps) could be shared between batched and scalar implementation. Refactored implementation of template<typename DataT, int WidthT> struct Masked; template<typename DataT> struct Ref; to flatten class hierarchy and fix bug where derivatives of unbounded arrays could not be constructed, IE: MaskedDx<int[]> myDeriv(ptr, arraylength); Added MaskedData::assign_val_lane_from_scalar Renamed MaskedData::assign_val_from to MaskedData::assign_val_from_wide Renamed MaskedData::assign_all_from to MaskedData::assign_all_from_scalar Updated BatchedAnalysis to indicate that results of pointcloud_search, pointcloud_get, and pointcloud_write are always varying regardless of the symbols passed in. Updated debugging in BatchedAnalysis to emit unmangled symbol names. Properly implement (was broken previously) BatchedBackendLLVM::llvm_zero_derivs(const Symbol& sym, llvm::Value* count) including using code gen loop to reduce code bloat. Signed-off-by: Alex M. Wells <[email protected]>
…and BatchedRendererServices<WidthT>::pointcloud_write not returning a value. Signed-off-by: Alex M. Wells <[email protected]>
Fixed undefined type when building with USE_PARTIO=0 Signed-off-by: Alex M. Wells <[email protected]>
…ed results and out of range max_points/num_points. Added support for NOOPTIMIZE file tag for batched regression tests. Made array-length-reg NOOPTIMIZE so its code gen would actually execute vs. constant fold. Fixed bug for storing arraylength result in varying result symbol. Signed-off-by: Alex M. Wells <[email protected]>
…n example-batched-deformer Signed-off-by: Alex M. Wells <[email protected]>
…id situations where point cloud could provice multiple data points at equal distances causing non-determinism. Signed-off-by: Alex M. Wells <[email protected]>
Signed-off-by: Alex M. Wells <[email protected]>
Signed-off-by: Alex M. Wells <[email protected]>
Signed-off-by: Alex M. Wells <[email protected]>
…ng back slightly different for essentially identical distances. Signed-off-by: Alex M. Wells <[email protected]>
…a parameter for a string. Signed-off-by: Alex M. Wells <[email protected]>
lgritz
approved these changes
Feb 19, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'm going to merge now so you can move on. There was one place I spotted an incorrect comment in testing.cmake, we can just fix that on the next PR.
Oh neat, if a committer expresses a review comment as a "suggestion", it lets you merge the suggestions directly! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Implement Batched pointcloud_search, pointcloud_get, and pointcloud_write.
Updated behavior of scalar pointcloud_search and pointcloud_get to not code gen branch with code gen runtime error for maxpoints/num_points larger than array size. Instead now it codgens a clamp of the maxpoints/num_points to the arraysize. This allows code to continue executing, and if accessed outside of the range, existing codgen range checks will still emit a runtime error.
Extracted class PointCloud definition and some helper functions to pointcloud.h so it can be shared between batched and scalar implementation.
Moved PointCloud out of anonymous namespace to OSL::pvt so a single implementation (including static maps) could be shared between batched and scalar implementation.
Refactored implementation of
template<typename DataT, int WidthT> struct Masked;
template struct Ref;
to flatten class hierarchy and fix bug where derivatives of unbounded arrays could not be constructed, IE:
MaskedDx<int[]> myDeriv(ptr, arraylength);
Added MaskedData::assign_val_lane_from_scalar
Renamed MaskedData::assign_val_from to MaskedData::assign_val_from_wide
Renamed MaskedData::assign_all_from to MaskedData::assign_all_from_scalar
Updated BatchedAnalysis to indicate that results of pointcloud_search, pointcloud_get, and pointcloud_write are always varying regardless of the symbols passed in.
Updated debugging in BatchedAnalysis to emit unmangled symbol names.
Properly implement (was broken previously) BatchedBackendLLVM::llvm_zero_derivs(const Symbol& sym, llvm::Value* count) including using code gen loop to reduce code bloat.
Fixed bug in llvm_gen for raytpye where the wrong type was passed as a name parameter . Only showed up when trying regression tests with optimizations disabled.
Tests
Expanded testsuite/pointcloud adding 2 additional write cloud tests and 10 additional read cloud including conditionally reading/writing and varying filename, maxpoints, and sort arguments.
Checklist: