Page MenuHomePhabricator

[OpenMP][IRBuilder] `omp task` support
ClosedPublic

Authored by shraiysh on Dec 29 2019, 11:20 PM.

Details

Summary

This patch adds basic support for omp task to the OpenMPIRBuilder.

The outlined function after code extraction is called from a wrapper function with appropriate arguments. This wrapper function is passed to the runtime calls for task allocation.

This approach is different from the Clang approach - clang directly emits the runtime call to the outlined function. The outlining utility (OutlineInfo) simply outlines the code and generates a function call to the outlined function. After the function has been generated by the outlining utility, there is no easy way to alter the function arguments without meddling with the outlining itself. Hence the wrapper function approach is taken.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
shraiysh updated this revision to Diff 422405.Apr 12 2022, 11:51 PM

Use correct method - getTypeStoreSize instead of getTypeSizeInBits.

Also, ping for review.

shraiysh updated this revision to Diff 423273.Apr 16 2022, 9:00 PM

Handle the case with no arguments to task function.

Herald added a project: Restricted Project. · View Herald Transcript
shraiysh updated this revision to Diff 423274.Apr 16 2022, 9:06 PM

Remove MLIR related code from this patch. (Added by mistake)

Thanks @shraiysh for taking up the work for task. This is the most important pending work for non-target OpenMP.

Could you expand the Summary a bit more with the approach taken and how it differs from current Clang task codegen?

I have added a few questions and comments.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1290

Do you know whether attributes are added to the outlined/wrapper function? Is it added in the finalize function? CreateParallel seems to have in in the PostOutlineCB.

1302–1307

Is this a change from clang codegen? Why not runtime_call(..., outlined_fn, ...)? Can you say that explicitly in the summary that this is different from Clang if it was changed for a particular reason?

1335

Nit: corresponding

1365

Is the size of shared a TODO? And would further changes impact the task size as well?
It seems by default variables are passed as private copies, but if the enclosing region has a data-sharing of shared then that should be honoured.

1391

Nit: A debug dump of the IR here might be useful.

1393–1394

Nit: The size can be omitted.

1395

Is this step required? TaskRegionBlockSet and Blocks do not seem to be used here, also collectBlocks function seems to be called from finalize as well.

FYI, I am revising how outlining works, still waiting on D115216 getting reviewed.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1313

Could you add an assert checking that there is just a single user, so we do not accidentally a wrong one.

1391

Please don't dump complete IR. Too much output makes -debug useless.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
1196–1198

[nit] only whitespace change

4452

[style] LLVM codsing style does not use Almost-Always-Auto.

4516–4521

Consider llvm::any_of

shraiysh edited the summary of this revision. (Show Details)Apr 26 2022, 4:37 AM
shraiysh updated this revision to Diff 425189.Apr 26 2022, 5:18 AM
shraiysh marked 11 inline comments as done.

Addressed comments

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1290

I have not added the attributes because I don't know about them and if they are required or not. I will check clang codegen once again for comments about any required attributes, but if you or anyone else knows what attributes are required and why, please let me know, I will add them.

1302–1307

The main reason for a difference is that Outlining generates a function call as shown in the input IR in the comment above. I could not find an easy way to change the arguments of a function after it has been constructed. Parallel does this by tweaking the CodeExtractor used inside outlining - and I did not want to touch outlining internally so that I can confidently rely on outlining to do its job.

A nice way to have similar (but not same) behavior as clang would be to add the inline attribute to the outlined function. After an Inline pass, the wrapper function will become the outlined function and it will be almost identical to clang's codegen. I will update the summary to highlight this difference.

1365

Yes, that is a todo, I have added it now. I think the task size will be reduced in that case. I have not looked into that yet and am doing private copies for everything at the moment.

1391

Please let me know if something less than IR should be dumped here.

1395

No it is not, I thought collectBlocks was required. I have removed it now.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
1196–1198

Removed. Thanks!

Meinersbur added inline comments.Apr 26 2022, 9:05 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1265–1275

I don't understand what this means.

1276

[style] Please use LLVM's naming standard.

1288

BodyGenCB may have invalidated AllocaIP and cannot be used anymore.

1302–1307

Could you add the function signature differences between wrapper_fn and outlined_fn? I.e. what arguments does wrapper_fn throw away?

Btw, current Clang emits such a wrapper function with -g as well, so I don't see it as an issue. LLVM will always inline a private function with just a single call site, except in -O0 where always_inline would be needed.

1333

Please document what HasTaskData. TaskSize, NewTaskData, etc are.

1353

Use a llvm::Twine

1368

What is flag 1?

1387–1388
1399

Please avoid temporary instructions. Use splitBB instead.

shraiysh updated this revision to Diff 426227.Apr 30 2022, 6:28 AM
shraiysh marked 7 inline comments as done.

Addressed comments. Rebase with main.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1265–1275

There are three basic block splits in the next few lines and this comment is meant to justify why three splits are needed - it basically tells which basic block will go where. Should I add something more to the comment?

1302–1307

The first argument is an i32 value but I do not know its usage. There is minimal documentation for task related stuff in OpenMP Runtime Library Reference in the OpenMP subproject. Here is the function signature for the wrapper function, and the first argument is discarded at the moment. If anyone has any reference to some documentation about this, or has any idea about what that argument represents, please let me know and I will try to handle it according to its purpose.

Btw, current Clang emits such a wrapper function with -g as well, so I don't see it as an issue. LLVM will always inline a private function with just a single call site, except in -O0 where always_inline would be needed.

Oh that is great then. Thank you!

1302–1307

Could you add the function signature differences between wrapper_fn and outlined_fn? I.e. what arguments does wrapper_fn throw away?

Btw, current Clang emits such a wrapper function with -g as well, so I don't see it as an issue. LLVM will always inline a private function with just a single call site, except in -O0 where always_inline would be needed.

1353

I have used it, but I am not sure if it is accurate usage. Please let me know if it seems inefficient.

1368

This is from flags in kmp.h. Value 1 means that it is tied (which is default unless untied clause is specified). Untied clause is not handled in this patch.

1399

AFAIK, splitBB requires an instruction pointer. I have updated this to erase the temporary instruction immediately after I have split the basic blocks, but I cannot figure out a way to completely eliminate temporary instructions. Is this okay?

Meinersbur added inline comments.May 2 2022, 4:51 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1352

Don't store a Twine in a variable. See comment from Twine.h:

/// A Twine is not intended for use directly and should not be stored, its
/// implementation relies on the ability to store pointers to temporary stack
/// objects which may be deallocated at the end of a statement. Twines should
/// only be used accepted as const references in arguments, when an API wishes
/// to accept possibly-concatenated strings.
1355

No SmallString<128> needed. str() creates a std::string that implicitly converts to a llvm::StringRef that is valid until the end of the statement(";").

Compared to using std::string only, this saves the creation of one temporary std::sting (for OutlinedFn.getName()). Your version saves another one (if fewer than 128 chars), but it is also more complicated. Before StringRef was made more compatible to std::string_view, the .str() wasn't even need.

1399
BasicBlock *TaskExitBB = splitBB(Builder, "task.exit");
BasicBlock *TaskBodyBB = splitBB(Builder, "task.body");
BasicBlock *TaskAllocaBB = splitBB(Builder, "task.alloca");

Note that the reverse order. After this, Builder will insert instructions before the terminator of Currbb (where you probably don't want to insert instructions here).

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
4501

If WrapperFunc is NULL, the next line will be a segfault. ASSERT_NE will stop execution if it fails.

4504

See above (and other occurances checking for nullptr)

4511–4514

Nice!

shraiysh updated this revision to Diff 426608.May 3 2022, 2:18 AM
shraiysh marked 10 inline comments as done.

Addressed comments

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1355

Alright, thanks for the explanation, I understand it better now.

Meinersbur added inline comments.May 3 2022, 8:31 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1265–1275

Consider something that resembles LLVM-IR syntax. Eg.

// def outlined_fn() {
//   task.alloca:
//     br label %task.body
//
//   task.body:
//     ret void
// }

outlined_fn is a view after the finalize call, but here we are constructing the CFG before outlining, i.e. the description does not match.

1324–1326

[serious] SrcLocStrSize cannot be passed-by-value and passed-bt-referenced at the same statement (unsequenced).

1333

Still don't see a description of TaskSize.

For HasTaskData, could you explain why and how the function signature changes?

1351–1352

WrapperFuncName and WrapperFuncNameStorage are now dead. Could you remove them?

1368

Please document that.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
4538

[style]

4539

[style]

shraiysh updated this revision to Diff 428285.May 9 2022, 9:43 PM
shraiysh marked 5 inline comments as done.

Addressed comments.

shraiysh updated this revision to Diff 428286.May 9 2022, 9:45 PM

Added description for Tied Argument

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1265–1275

outlined_fn is a view after the finalize call, but here we are constructing the CFG before outlining, i.e. the description does not match.

Yes, that was supposed to justify why we need four splits here. Without this comment it wasn't really clear to me why three blocks won't be enough (currBB, task.body and task.exit). It also isn't directly intuitive that task.exit is not going to be a part of the outlined function. I have added that this is going to be the basic block mapping after outlining. If it seems unnecessary, please let me know and I will remove this comment.

1333

I have added that TaskSize refers to the argument sizeof_kmp_task_t in the runtime call. I did not want to add further information because the argument's exact meaning is not documented anywhere (the reference doesn't have it). My interpretation of the argument is the size of arguments in bytes, but that could be incorrect and I thought it was better to redirect anyone reading this/working on this to the argument of the runtime call instead of writing a possibly misleading description. Please let me know if it would be better to add the current interpretation of the argument.

For HasTaskData, could you explain why and how the function signature changes?

Documented this near the wrapper function. Please let me know if it requires some alteration.

Ping for review.

Meinersbur accepted this revision.May 19 2022, 8:21 AM

LGTM. Consider adding some description of TaskSize before committing.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1333

https://github.com/llvm/llvm-project/blob/a4190037fac06c2b0cc71b8bb90de9e6b570ebb5/openmp/runtime/src/kmp_tasking.cpp#L1345

// sizeof_kmp_task_t:  Size in bytes of kmp_task_t data structure including
// private vars accessed in task.
This revision is now accepted and ready to land.May 19 2022, 8:21 AM
shraiysh updated this revision to Diff 431241.May 22 2022, 9:08 AM
shraiysh marked 3 inline comments as done.

Thank you @Meinersbur for pointing out the description of the argument, I had not checked the cpp file. I have added the description now.

Addressed comments. I will wait for two more days, and if there are no further comments on this patch, then I will land it.

Meinersbur accepted this revision.May 23 2022, 1:48 PM
This revision was landed with ongoing or failed builds.May 23 2022, 9:52 PM
This revision was automatically updated to reflect the committed changes.