Page MenuHomePhabricator

[OMPIRBuilder] Fix static schedule in worksharing-loop directive
AbandonedPublic

Authored by peixin on Sun, Nov 21, 12:22 AM.

Details

Summary

If no chunk_size is specified, OMPScheduleType for kmpc_for_static_init
call is Static and the chunk size is set to 1. If chunk_size is specified,
OMPScheduleType for kmpc_for_static_init call is StaticChunked. Also
fix lowering to LLVM IR for schdule clause in worksharing-loop directive.

Diff Detail

Event Timeline

peixin created this revision.Sun, Nov 21, 12:22 AM
peixin requested review of this revision.Sun, Nov 21, 12:22 AM
peixin edited the summary of this revision. (Show Details)Sun, Nov 21, 12:24 AM

Generally looks good to me. Just one nit-pick and one "I don't really understand how this actually produces the correct result" (may be that I'm just missing something!)

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
726

I may be a bit confused here, but isn't this setting chunk to not-null, and then expecting the apply function to see it as null?

mlir/test/Target/LLVMIR/openmp-llvm.mlir
457

I'd like to see this having a separate name - it makes it so much easier to search the source code when the tests have distinct names! :)

peixin updated this revision to Diff 389081.Mon, Nov 22, 7:06 PM

Change the variable name"const_i32" to "static_chunk_size" and remove redundant code.

peixin added inline comments.Mon, Nov 22, 7:16 PM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
726

This code is the "else" branch, which is for non-static schedule. As OpenMP 5.1 [2.11.4] states, the chunk_size defaults to 1 when it is unspecified for dynamic or guided schedule. The chunk size cannot be specified for runtime or auto schedule. But in implementation, llvm openmp runtime uses kmpc_dispatch_init with the argument "chunk_size" of 1 for runtime or auto schedule.

To be short, schedule(static) is NOT the same as schedule(static,1).
schedule(dynamic) is the same as schedule(dynamic,1).
schedule(guided) is the same as schedule(guided,1).
schedule(runtime, 1) and schedule(auto, 1) is not allowed, but schedule(runtime) and schedule(auto) behave like schedule(runtime, 1) and schedule(auto, 1) in IR level.

I noticed "if (!chunk) chunk = one;" is also added in applyDynamicWorkshareLoop in OMPIRBuilder, so remove this code here.

Meinersbur requested changes to this revision.EditedMon, Nov 22, 8:21 PM

@peixin is correct. However, a "dispatch" loop is needed in addition to a "chunk" loop, please see TODO in the description of applyStaticWorkshareLoop. This patch would only iterate over chunk_size*omp_get_num_threads() iterations and leave any additional loop iterations unprocessed.

Implementation of static schedule without chunk-size: D114413

This revision now requires changes to proceed.Mon, Nov 22, 8:21 PM

@Meinersbur Thanks for the clarification. Can you also handle the fix in mlir part in D114413 (initilize chunk to nullptr and add some test cases in oepnmp-llvm.mlir)? I will close this PR since it is not complete fix.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
726

Thanks, I obviously got confused by the "hidden" bit of code, and thought it was just continuation of the static wsloop code.

@Meinersbur Thanks for the clarification. Can you also handle the fix in mlir part in D114413 (initilize chunk to nullptr and add some test cases in oepnmp-llvm.mlir)? I will close this PR since it is not complete fix.

See update to D114413