Minor cleanup of parallel refactor intrinsics #6465
Merged
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
- Renamed `load_struct_member` to `load_typed_struct_member` to make it more clear that it is intended for use only with the results of `make_typed_struct`. - Split `declare_struct_type` into two intrinsics, `define_typed_struct` and `forward_declare_typed_struct`, removing the need for the underdocumented `mode` argument and hopefully making usage clearer - Added / clarified comments for the intrinsics modified above
dsharletg
approved these changes
Dec 2, 2021
zvookin
reviewed
Dec 3, 2021
| @@ -2550,13 +2562,19 @@ void CodeGen_C::visit(const Call *op) { | |||
| rhs << "(" << print_type(op->type) << ")"; | |||
| } | |||
| rhs << "(&" << struct_name << ((count > 0) ? "[0])" : ")"); | |||
| } else if (op->is_intrinsic(Call::load_struct_member)) { | |||
| } else if (op->is_intrinsic(Call::load_typed_struct_member)) { | |||
| // Given an instance of a typed_struct, a definition of a typed_struct, | |||
There was a problem hiding this comment.
Given all the confusion around a value that solely exists to communicate a type, this comment should probably spend a few words on what an "instance of a typed struct" is. I.e. a Handle type that was previously returned from one of the intrinsics that returns one.
zvookin
reviewed
Dec 4, 2021
src/CodeGen_C.cpp
Outdated
| // An instance of a typed_struct is an Expr of Handle() type that has been | ||
| // created by a call to make_typed_struct. | ||
| // | ||
| // An definiton of a typed_struct is an Expr of Handle() type that has been |
There was a problem hiding this comment.
change "An" to "A".
It is probably worth covering that both are needed because the instance is often a void * by the time it is being used. E.g. it has been passed through an API that takes an opaque pointer as a void *. Thus it needs to be casted back to the correct type so its fields can be accessed.
zvookin
reviewed
Dec 4, 2021
src/CodeGen_LLVM.cpp
Outdated
| // An instance of a typed_struct is an Expr of Handle() type that has been | ||
| // created by a call to make_typed_struct. | ||
| // | ||
| // An definiton of a typed_struct is an Expr of Handle() type that has been |
There was a problem hiding this comment.
Ditto comments on version in CodeGen_C.cpp . I find this method of documenting intrinsics highly dubious precisely for the reason that it is duplicated and thus a comment this long is basically unhelpful. Probably worth a TODO and an issue to move all intrinsics documentation into a header file with the intrinsics themselves.
zvookin
approved these changes
Dec 4, 2021
steven-johnson
added a commit
that referenced
this pull request
Dec 14, 2021
…R lowering pass. (#6195) * First cut at factoring parallel task compilation, including closure generating and calling, into a normal IR to IR lowering pass. Includes adding struct handling intrinsics to LLVM and C++ backends. Still a work in progress. * Fix formating that got munged by emacs somehow. * Checkpoint progress. * Small fixes. * Checkpoint progress. * Checkpoint preogress. * Checkpoint progress. * Checkpoint progress. Debugging code will be removed. * Try a fix for make_typed_struct in C++ codegen. * Another attempt to fix C++ codegen. * Another C codegen fix. * Checkpoint progress. * Use make_typed_struct rather than make_struct to construct closure. Ensure all types are carried through exactly the same to both make_struct_type and make_typed_struct. * Checkpoint. * Uniqueify closure names because LLVM was doing that to function names. * Small formatting cleanups. Fixes to call graph checker. Disable tests related to this while Andrew and I figure out how to get it to work across closures. * Get generated C++ to compile via a combination of fixing types and bludgeoning types into submission via subterfuge. * Typo fix to a typo fix. * Restore inadvertently deleted code. * Rename make_struct_type to declare_struct_type. * Add new file to CMake. * Add fixes for Hexagon offload and any passes that might add additional LoweredFunctions in the future. * Add comment with a bit of info for the future.. * Typo fix. * Don't duplicate the closure call to test the error return. Don't declare halide_buffer_t type if there are no buffers in closure. * Use _ucon in C++ code to get rid of constness casting ugliness. * Change resolve_function_name intrinsic to use a Call node to designate the function. This makes generating the C++ declaration in the C++ backend trivial. Few more changes to type handling and naming. * Small C++ backend output formating change. Don't generate For loops with no variable. Update internal test for C++ output. * Add halide_semaphore_acquire_t as a well known type for use inside compiler. * Add handling for halide_semaphore_t allocation. Formating fixes. * Fix type for halide_semaphore_t. * Reapply C++ backend formatting fix. * Add support for calling legacy halide_do_par_for runtime routine in cases where it is valid. * Formatting fixes. * Format and tidy fixes. * Attempt to pass formatting check. * Fix last set of test failures. * Formatting whitespace fixes. * Update comments. * Attempt to fix pointer cast error with some versions of LLVM. * Another attempt at fixing bool compatibility casting. * Another iteration. * Remove likely useless extern argument check logic. * Add hacky fix for losing global variables. * Comment typo fixes. * Remove no-longer-used Closure code from Codegen_Internal * Remove unused MayBlock visitor class * clang-tidy * Attempt to fix parallel offloads for HVX * Update parallel_nested_1.cpp * Augment Closure debugging * Add some std::move usage * Fix hvx lock/unlock semantics for PR #6457 (#6462) Fix qurt_hvx_lock issues * Sort IntrinsicOp and corresponding names * Remove unused `is_const_pointer()` function * Minor hygiene in LowerParallelTasks - normalize local functions to snake_case - put all local functions & classes in anon namespace - move MinThreads visitor to file scope to reduce nestedness of code * use Closure::include * Switch to PureIntrinsics per review feedback. * Minor cleanup of parallel refactor intrinsics (#6465) * Minor cleanup of parallel refactor intrinsics - Renamed `load_struct_member` to `load_typed_struct_member` to make it more clear that it is intended for use only with the results of `make_typed_struct`. - Split `declare_struct_type` into two intrinsics, `define_typed_struct` and `forward_declare_typed_struct`, removing the need for the underdocumented `mode` argument and hopefully making usage clearer - Added / clarified comments for the intrinsics modified above * Update comments * Fix comments * Update CodeGen_C.cpp * Remove 'foo.buffer' from Closure entirely This is a direct adaptation of what #6481 does for the old LLVM-based code, and allows elimination of one use of `get_pointer_or_null()`. PR is meant to be merged into factor_parallel_codegen, not master. * Update LowerParallelTasks.cpp * Keep track of task_parent inside LowerParallelTasks; remove no-longer-needed get_pointer_or_symbol() intrinsic (#6486) * Fix potential issue with additional LoweredFuncs (#6490) I think this is the right fix for this case; that said, I can't find a single instance in any of our test cases that actually triggers this. * factor parallel codegen with fewer intrinsics (#6487) * Rework some of parallel closure lowering to avoid some intrinsics This version relies more heavily on the existing make_struct, and puts function names in the Variable namespace as globals. Co-authored-by: Steven Johnson <srj@google.com> Co-authored-by: dsharletg <dsharlet@google.com> Co-authored-by: Steven Johnson <srj@google.com> Co-authored-by: Andrew Adams <andrew.b.adams@gmail.com>
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.
load_struct_membertoload_typed_struct_memberto make it more clear that it is intended for use only with the results ofmake_typed_struct.declare_struct_typeinto two intrinsics,define_typed_structandforward_declare_typed_struct, removing the need for the underdocumentedmodeargument and hopefully making usage clearer