This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][OpenMP] Add support for basic SIMD construct
ClosedPublic

Authored by arnamoy10 on Jan 24 2022, 12:31 PM.

Details

Summary

Patch adds a new operation for the SIMD construct. The op is designed to be very similar to the existing wsloop operation, so that the CanonicalLoopInfo of OpenMPIRBuilder can be used.

Diff Detail

Event Timeline

arnamoy10 created this revision.Jan 24 2022, 12:31 PM
arnamoy10 requested review of this revision.Jan 24 2022, 12:31 PM

You may need one test case in mlir/test/Target/LLVMIR/openmp-llvm.mlir.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
317

2.9.3 is for simd directives. I think you are implementing simd contruct [2.9.3.1]. No construct called simd loop contruct. In addition, simd loop is kind of confusing since someone may think of it as worksharing-loop simd contruct.

shraiysh requested changes to this revision.Feb 24 2022, 9:24 AM
shraiysh added subscribers: clementval, shraiysh.

Thanks for the patch @arnamoy10. Requesting a few changes.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
348

The current head has removed such definitions of parsers/printers in favor of fields hasCustomAssemblyFormat and hasVerifier (It is a minor change). Can you please rebase and use them?

Another point is, can we use assemblyFormat for this? We can have a custom parser/printer for loop bounds if assembly format does not allow separating region arguments from the region itself. (@kiranchandramohan @clementval FYI, I was planning on doing the same with WSLoop assemblyFormat.) Thoughts?

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

There are no test for this. The tests should be added to mlir/test/Target/LLVMIR/openmp-llvm.mlir.

889

Can we instead move it to the verifier right away instead of adding a TODO.

892–897

There is a constructor for LocationDescription that just takes an IRBuilderBase. That can be used here.

This revision now requires changes to proceed.Feb 24 2022, 9:24 AM

Also, please run git-clang-format HEAD~ before submitting it to eliminate formatting related errors.

arnamoy10 updated this revision to Diff 412106.Mar 1 2022, 7:47 AM

Addressing reviewers' comments.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
317

I think SimdLoopOp is OK. for worksharing-loop simd contruct, we can call the Op WsSimdLoopOp

348

Thanks, did the mods. For Assemblyformat addition I will wait until your patch for the wsloop

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

Thanks, added.

892–897

Thanks for the suggestion. However, I will be needing the diLoc in later code, therefore I need to keep the subprogram and diLoc variables.

peixin added inline comments.Mar 1 2022, 7:37 PM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
317

OK. Agree.

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 7:37 PM

Thanks for addressing the comments @arnamoy10. Generally looks good. A few nits and comments.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
328

Nit: This line has over 80 characters.

356–357

Nit: This line has over 80 characters.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
1020

Nit: Can we please put this closer to the parser/printer?

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

I think we can safely remove this from here now, as it will be checked in verifier.

892–897

You can use ompLoc.DL for that as done here.

mlir/test/Dialect/OpenMP/ops.mlir
317

Can we please have one test with multiple arguments in this syntax? More than one (%lb, %ub and %step).

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

Same here. Can we please have one test with multiple arguments in this syntax? More than one (%lb, %ub and %step).

arnamoy10 updated this revision to Diff 413516.Mar 7 2022, 9:13 AM

Addressing reviewers' comments

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
328

No, just 78. It is also clang-formatted

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
1020

Done, thank you

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

Done, thank you

892–897

Thanks, done

mlir/test/Dialect/OpenMP/ops.mlir
317

Thanks done

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

Thanks, done

shraiysh accepted this revision.Mar 9 2022, 1:32 AM

LGTM.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
328

I still see it as 82 characters (78 text + 4 spaces at the beginning). Unfortunately, clang-format does not format tablegen files so we have to do this manually.

This revision is now accepted and ready to land.Mar 9 2022, 1:32 AM
arnamoy10 updated this revision to Diff 415194.Mar 14 2022, 1:05 PM

Rebase on main and fix line character count

This revision was automatically updated to reflect the committed changes.