This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by shraiysh on Mar 23 2022, 1:13 AM.

Diff Detail

Event Timeline

shraiysh created this revision.Mar 23 2022, 1:13 AM
Herald added a project: Restricted Project. · View Herald Transcript
shraiysh requested review of this revision.Mar 23 2022, 1:13 AM
shraiysh updated this revision to Diff 417531.Mar 23 2022, 1:54 AM

Fixed spelling error.

ftynse accepted this revision.Mar 23 2022, 3:05 AM
This revision is now accepted and ready to land.Mar 23 2022, 3:05 AM
kiranchandramohan added inline comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
642

The code here is probably very similar to the master translation, is there any scope for codesharing?

655

Nit: Can you add a comment for the last field (which is nullptr)? Is. that required for copyprivate?

shraiysh updated this revision to Diff 417642.Mar 23 2022, 8:43 AM

Addressed comments.

shraiysh added inline comments.Mar 23 2022, 8:46 AM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
642

We can do that, but there are two things we have to think about:

  1. Will it make it harder to understand and modify the code, because master and single are two unrelated constructs that have one function.
  2. Master has no clauses, while single has clauses (the handling for allocate and nowait has not been added because it has not been handled by the IRBuilder yet, but they will be added) and will having a common function make it harder to handle both?

I suppose with careful handling, we could carve out a part of both these functions into one function, but intuitively what will that function will doing (what would be it's name)? I can give it a shot if you'd like to see how that would look like. Please let me know.

655

Sure, I will add it. I am not too sure what that is for. It is used here to set the value to i32 0.

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

OK. Probably not worth it.

This revision was automatically updated to reflect the committed changes.