Page MenuHomePhabricator

[mlir][OpenMP] omp.task translation to LLVM IR
ClosedPublic

Authored by shraiysh on Apr 17 2022, 9:36 PM.

Details

Summary

This patch adds translation for omp.task from OpenMPDialect to LLVM IR
Dialect and adds tests for the same.

Depends on D71989

Diff Detail

Event Timeline

shraiysh created this revision.Apr 17 2022, 9:36 PM
Herald added a project: Restricted Project. · View Herald Transcript
shraiysh requested review of this revision.Apr 17 2022, 9:36 PM
ftynse accepted this revision.Apr 20 2022, 11:51 PM
ftynse added inline comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
682

Nit: there is no need to pass MLIR ops by reference.

This revision is now accepted and ready to land.Apr 20 2022, 11:51 PM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
697

Nit: spell out this auto.

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

Nit: are x, y, zaddr used to check that there are no arguments for the wrapper function and hence the task data size is 0?

2187

Nit: Is the alloca significant for your test? Do you want to check that it is in the outlined region?

2226

Why is the task size 16 and not 12 (for the three liveins)?

shraiysh updated this revision to Diff 424815.Apr 24 2022, 9:32 PM
shraiysh marked 4 inline comments as done.

Addressed comments.

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

Yes, nothing from outer scope should be counted for tasksize, I will change it to just %x and %zaddr if you'd like.

2187

Yes, added check for it. Thanks for pointing this out.

2226

x and y are 4-byte each, and the pointer zaddr is of 8 bytes (64-bit). So, the total is 16.

shraiysh updated this revision to Diff 431883.May 24 2022, 10:28 PM

Rebase with main. Requesting review again as it has been a while since this was accepted.

shraiysh requested review of this revision.May 24 2022, 10:28 PM

A bit-ness question

A bit-ness question

I didn’t understand this comment. Is this a typo or incomplete comment?

Ping for review!

LG. Please wait a day before submitting.

This revision is now accepted and ready to land.May 31 2022, 7:27 AM
peixin accepted this revision.May 31 2022, 7:57 AM

LGTM

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1329–1330

Nit: move "call." to line 1328.

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

Nit

691

Is allocaIP used in this?

Meinersbur added a comment.EditedJun 1 2022, 10:58 AM

A bit-ness question

I didn’t understand this comment. Is this a typo or incomplete comment?

Sorry, I added an inline-comment but Phabricator refused to submit without also adding something in the main comment box. Now, I don't see the inline-comment so that may be the reason Phabricator complained.

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

A pointer is 8 bytes only on 64-bit platforms. Does this mean that this test will fail on platforms that have pointers of different sizes? I also don't see the target triple fixed by mlir-translate, so will this test fail on non-64 hosts?

shraiysh added inline comments.Mon, Jun 6, 8:57 AM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
691

It is not used in this instance, but it is required in OpenMPIRBuilder for the most general case and so this is required.

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

It could. I don't know who should set the target-triple/data-layout. Should it be a command line option for mlir-translate? A similar test is there in OpenMP IRBuilder here, which could cause issues (it hasn't caused any so far) and I should probably rectify that too.

Meinersbur added inline comments.Mon, Jun 6, 2:08 PM
mlir/test/Target/LLVMIR/openmp-llvm.mlir
2226

The OpenMPIRBuilder tests use an empty TargetTriple ("") which represents an 64-bit architecture with byte-aligment. What does mlir-translate do?

shraiysh updated this revision to Diff 434897.Tue, Jun 7, 11:22 AM

Added target triple to make sure that the test doesn't fail on non-x86 machines.

shraiysh added inline comments.Tue, Jun 7, 11:30 AM
mlir/test/Target/LLVMIR/openmp-llvm.mlir
2226

I have added the target triple to the testcase now. This should pass it on other machines. Does this look okay now?

@Meinersbur does this patch look okay now? I wanted to merge this, if there are no issues.

If there are no further issues, I will merge this tomorrow. It would be nice to have E2E task support.

Meinersbur accepted this revision.Thu, Jun 30, 1:31 PM

LGTM, thank you.

This revision was landed with ongoing or failed builds.Mon, Jul 4, 8:33 AM
This revision was automatically updated to reflect the committed changes.