HexagonOffload needs to call inject_hvx_lock_unlock() before lower_parallel_tasks() #6457
Conversation
|
I am not sure this is the right fix. I thought the behavior at master was:
However, I can't find the implementation of this. The thread pool/halide_do_par_for implementation has been refactored significantly since I remember implementing this. I think after @zvookin's PR, the behavior instead should be simplified, because now we should be locking HVX every time we enter a parallel loop body, i.e. we should be able to simplify the code. I suspect this may be buggy, because it's trying to re-lock HVX on the same thread where it is already locked. The solution that is probably to unlock HVX prior to calling However, I now suspect that on master, we simply aren't calling qurt_hvx_lock at all on worker threads! I'm wondering if maybe we should just stop calling qurt_hvx_lock entirely? @pranavb-ca @aankit-ca IIRC locking HVX became optional at some point? |
|
Actually, my suggestion of unlocking + relocking might not be safe, because maybe we can capture vector variables in the parallel loop closure? Maybe these vectors would be lost if we unlocked + relocked HVX? |
|
In theory, the behavior with this fix should be identical to what we did before; we're just doing the transformation to insert locks earlier, and the lock/unlock calls get redistributed along with the rest of the code in the loop. That said, this was already a messy thing that was hard to understand, so if this allows us to simplify the situation, I'm all for it -- I'm just not sure what the needed semantics for the lock/unlock are. |
steven-johnson
changed the base branch from
factor_parallel_codegen
to
master
|
(converted to draft so I can change the base to master to enable proper testing; don't land it this way) |
|
OK, here's the logic I was looking for: Halide/src/CodeGen_Hexagon.cpp Lines 333 to 355 in ed87acb The thing I missed is that it was moved from runtime logic to codegen logic at some point. |
Right, and that's (still) what we're doing here, so I think we should be alright. That said, it may be possible to simplify this down further; I will take a look at doing so tomorrow and try to offer an alternate fix. |
|
@dsharletg @pranavb-ca @aankit-ca Are the specific rules for when |
|
Hey @steven-johnson give me some time to get back to you. I'll try this afternoon to see if we can get rid of this call altogether. (Sorry for the delayed response, I have been out of the office for the last two weeks. I am out until tomorrow but might be able to try this this afternoon nonetheless) |
|
I think we should keep the calls for now and work on removing them separately. I think what we should do for now is that Zalman's refactor will insert qurt_hvx_lock calls everywhere needed. The problem is we lost the ability to unlock HVX prior to calling halide_do_par_for. The existing logic (and this PR preserves this) tries to be clever and avoid unlocking/relocking when possible. I think maybe we should just unconditionally unlock HVX and relock HVX (if needed) prior to any halide_do_par_for call. I doubt the overhead/cost of this will be significant relative to loops that are parallelized. |
Not sure I understand -- AFAICT, Zalman's refactor doesn't attempt to do anything like this right now, are you suggesting we modify it to do so?
Yeah, but it seems to have been working for quite a while now. Is there a downside to re-using it that I'm overlooking? Lack of verifiability? |
steven-johnson
changed the base branch from
master
to
factor_parallel_codegen
Zalman's PR causes the body of parallel loops to be a separate function, which get qurt_hvx_lock calls inserted by CodeGen_Hexagon just like any other function.
Yes, it's complicated, and hard to be sure it is working as intended. |
Ah, ok, I see now a case where we do end up with a double-lock getting inserted by this. (Oddly, the test still passes.) I'll withdraw this and try your suggestions. |
Speaking of which -- it looks like we currently don't attempt to do any of the locking for HVX code that uses async (i.e. |
…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>
Not 100% sure this is the proper fix, but it's definitely the issue we are seeing with correctness_parallel_nested_1; the qurt_hvx_lock stuff only got injected for parallel For loops, but with the changes in factor_parallel_codegen, those are all gone by the time we hit that code. Solution here is to explicitly call inject_hvx_lock_unlock() on blocks that we are lowering as parallel tasks in HexagonOffload, but I need to convince myself we aren't inserting duplicate locks or unlocks now (or just getting lucky).