This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][OpenMP] Support schedule chunk size with various bit width
ClosedPublic

Authored by peixin on Dec 20 2021, 7:28 PM.

Details

Summary

The chunk size in schedule clause is one integer expression, which can
be either constant integer or integer variable. Fix schedule clause in
MLIR Op Def to support integer expression with different bit width.

Diff Detail

Event Timeline

peixin created this revision.Dec 20 2021, 7:28 PM
peixin requested review of this revision.Dec 20 2021, 7:28 PM

Hi @peixin. Thanks for the patch! I had a few comments.

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

As we are taking type of chunkVar now, can it be unsigned integer too? (In other words, is it possible that we need zext/sext here?).

649

Is there a specific reason why we are truncating chunkVar and not extending step in this branch? (seems like some information could be lost while truncating).

mlir/test/Dialect/OpenMP/ops.mlir
220

Maybe modify type to i16 or i64 in one of these to make sure we accept other types too in roundtrip.

mlir/test/Target/LLVMIR/openmp-llvm.mlir
466–469

It looks like we are not checking anything about the i16 type. In other words, even if the lowering completely ignores the i16 type, this test will pass. Can we check something about proper handling of i16 deterministically here (sext or trunc instructions maybe)? If not, then this basically checks parsing and that is already checked in ops.mlir.

480

This test looks exactly the same as the previous one - test_omp_wsloop_dynamic_chunk_var. Can you please check this once?

peixin updated this revision to Diff 395780.Dec 21 2021, 5:46 PM

Fix test case.

Thanks @shraiysh for the review. Addressed the comments.

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

The chunk_size must be a loop invariant integer expression with a positive value. So it does not matter if we use zext or sext.

649

Yes. The ivType, the types of lower bound, upper bound, and step are implemented in https://github.com/flang-compiler/f18-llvm-project/pull/1256. The ivType must be i32 or i64. The max value with i32 type is definitely enough for chunk size. Users won't use the chunk size greater than INT_MAX, so truncating won't lose the information.

mlir/test/Dialect/OpenMP/ops.mlir
220

Thanks for the notice. Fixed.

mlir/test/Target/LLVMIR/openmp-llvm.mlir
466–469

The LLVM IR geneartion will fail if ignoring the i16 type. I think checking the data type of the last element of kmpc_dispatch_init_4u is enough. Anyway, I add the sext instruction check.

480

Thanks for the notice. I had planned to check truncating i64 to i32 here. Fixed.

@shraiysh Can you accept this PR?

shraiysh accepted this revision.Jan 17 2022, 10:51 AM

Apologies for not accepting this earlier.

This revision is now accepted and ready to land.Jan 17 2022, 10:51 AM
peixin updated this revision to Diff 400786.Jan 18 2022, 3:02 AM

@shraiysh Thanks a lot.

Rebase to test CI before landing this patch.