This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.
ClosedPublic

Authored by fghanim on Jan 6 2020, 2:53 PM.

Details

Summary

Add support for Master and Critical directive in the OMPIRBuilder. Both make use of a new common interface for emitting inlined OMP regions called emitInlinedRegion which was added in this patch as well.

Also this patch modifies clang to use the new directives when -fopenmp-enable-irbuilder commandline option is passed.

Event Timeline

fghanim created this revision.Jan 6 2020, 2:53 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 6 2020, 2:53 PM

Some initial comments. Can we split it in two patches (master/critical)?

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
249

If there is no good reason agains it, these should go into the OMPKinds.def/OMPConstants.{h/cpp}. We need support for array types but that is not a problem.

258

A lot of the functions below should not be public. We should expose as little as possible, mainly the CreateDirective methods.

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

You can just write OMPD_master below instead of OMPD.

651

Make sure the patch is clang formatted. See also: https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

845

Is this switch + if-cascade + callEntry/Exit really helpful?
I mean we can replace

`Instruction *EntryCall = CreateEntryCall(OMPD, Args);`

with

`Instruction *EntryCall = Builder.CreateCall(getOrCreateRuntimeFunction(OMPRTL___kmpc_omp_master), Args);`

right?

fghanim marked 5 inline comments as done.Jan 7 2020, 2:47 PM

I don't think there is enough in this patch to split it into two or three patches. The main part is EmitOMPInlinedRegion which does all the heavy lifting. At this point, both create Master & Critical are almost wrappers collecting arguments for EmitOMPInlinedRegion. So I really do not want to split them into multiple patches unless I have to.

As for the rest , I just wanted to provide my thinking when I did them. I'll update them if you think it's better to. let me know.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
249

I tried to do that, but started to get compilation errors when I put them in OMPConstants.{h/cpp} . So I thought that given that these are used exactly once according to current implementation in clang (i.e. for 'critical name'). I'll do it as part of OMPBuilder for the time being.

258

I think the OMPbuilder should provide people writing llvm transformation passes (e.g. autoparallelizing) with a way to be able to emit correct OMP calls based on their needs.

If you have minor suggestions to make it better, I'd love to update this based on that.
Alternatively, I'll make them private for now, until we decide on something for that.

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

Yes. I just feel this is a bit cleaner is all.

651

Will do

845

While I completely agree that the switch+if-cascade is not an elegant solution, It's helpful in enabling a generic way to the directive entry and exit calls. which is in turn helpful for EmitCommonDirectiveEntry and EmitCommonDirectiveExit.

FWIW, CreateEntryCall was used inside of EmitCommonDirectiveEntry, similar to how CreateExitCall is currently inside EmitCommonDirectiveExit. RTLFuncName enabled that. I only took it out because of Critical's Entry_Hint special case. I plan to put it back in, If critical is the only directive like that.

I am not too attached to it, but I think it's help-ing :)

jdoerfert added inline comments.Jan 7 2020, 3:09 PM
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
249

These are not good arguments. We have *all other types* defined in OMPKinds.def + OMPConstants.{h/cpp}, doing something different here just complicates the code.

258

It is futile to design based on some use cases we cannot describe properly. Once the need arises for public interfaces, the callType (inconsistent spelling!), the Entry/Exit call stuff, we can add it but not before.

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

I argue it is harder to read the code this way (especially at the call site below). Though, I won't make the change a requirement.

845

What is EmitCommonDirectiveEntry and EmitCommonDirectiveExit ?

I don't see how it is helping to have emitEntry(createEntry(DIRECTIVE)) instead of emit(DIRECTIVE_ENTRY), especially since the former needs all this logic to map DIRECTIVE to DIRECTIVE_ENTRY. The latter shows you at the call site, e.g., in CreateCritical, exactly what runtime call is being created and at these call sites we already know exactly what runtime call we need.

You can still have some helper that puts entry/exit around some code, simply pass both the entry and exit runtime call ID instead of just the directive ID from which you would "compute" the entry/exit runtime call ID.


Long story short, if making things explicit minimizes code complexity along the way I'm all for it.

jdoerfert added inline comments.Jan 9 2020, 8:54 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
840

While I hope this code goes away, I should mention that we don't write assert(false && "...") but llvm_unreachable("...").

fghanim updated this revision to Diff 237149.Jan 9 2020, 12:00 PM
  • Adding array types to OMPKinds.def. Inlining runtime function calls generation. reformatting with clang format
fghanim marked 8 inline comments as done.EditedJan 9 2020, 12:05 PM

So I modified the patch based on your comments. I removed all the code that I intended for llvm pass writers for now.
I will look at it again later, figure something to do with the whole switch-if-cascade thing, and will introduce it in a separate patch. I think that would be better than to include it here.

I added quite a bunch of inline comments again :( and some high level stuff below.


Commits need a message that explains the change.
Also the "Summary" should not be in the commit title.


This adds support for clang to build the directives via the OMPIRBuilder so we need to add tests for that. Probably "just" running existing codegen tests for the directives with the OMPIRBuidlder enabled.

clang/lib/CodeGen/CGStmtOpenMP.cpp
3005

Can you add an assertion that the branch has only a single successor.

3009

IPBBTI->eraseFromParent()

3020

Why the isSet check? If we need it, then it's missing in other places as well (I think).

3060

Can we do

llvm::Value *HintInst = nullptr;
if (Hint)
  ...

these three lines look ugly :(

3076

I somehow have the feeling we could have a "FiniCB" helper function as they seem to be always the same. A TODO mentioning that is also OK for now.

3099

Same here, that looks like the other "BodyGenCB". We should really make them helpers.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
259

FiniCB param docu is missing.

295

Conditional

312

HasFinalize

331

Format: no newline, above the comments got broken

349

All methods need documentation of some kind. llvm:: is not needed here. It seems one method is declared twice.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp
68

These comments that reference KmpCriticalNameTy do not help here. If you want, feel free to add comments that explain how the result looks in a generic way but only showing KmpCriticalNameTy seems counterproductive to me.

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

Spelling: Fn

695

Spelling Conditional, HasFinalize

700

Outdated comment.

717

Nit: InsertPointTy FiniIP(FiniBB, FiniBB->getFirstInsertionPt());

721

Shouldn't we use the argument here?

728

Style: No braces

737

Conditional, here and elsewhere no llvm:: (and omp::)

739

Naming: ContPt or ContIP or ...

766

I usually have neither or both conditional branches in braces. Here we can opt for an early exist (preferred anyway) and get:

if (!Conditional)
  return Builder.SaveIP();
...

Some variables can then be moved past this early exit

790

extra parenthesis, and, actually, this condition can not happen (I think). There is always something at the end of the block or your get an error earlier.

800

ExitCall->moveBefore(IP), potentially with some IP = ... above.
We should specify what the builder insert position is after this call, maybe "unspecified"?

808

This can be a static helper and the name should be more descriptive, getName is very generic.

824

This seems to be too much trouble to get a StringRef from a Twine. Maybe start with a StringRef and avoid all of it?

836

Do we really want common linkage for "internal variables"? Internal or private would have been my first guess. Maybe the "internal" part is different than I expect it.


FWIW, the usual pattern is

if (!Elem.second)
  Elem.second = ...
assert(...)
return Elem.second;
fghanim marked 10 inline comments as done.Jan 11 2020, 10:18 PM

Commits need a message that explains the change.
Also the "Summary" should not be in the commit title.

Is this something for future commits or do you want me to change these?
If the latter, how do I do that?

This adds support for clang to build the directives via the OMPIRBuilder so we need to add tests for that. Probably "just" running existing codegen tests for the directives with the OMPIRBuidlder enabled.

You mean unit tests? or something else? If not unit tests, Where do I do that?

As for rest of comments, anything without response, is something I will do.

clang/lib/CodeGen/CGStmtOpenMP.cpp
3020

While I was writing the response I realized that I should've made it
assert((!AllocaIP.isSet() || AllocaInsertPt == AllocaIP.getpoint()) && "Inlined directives allocas should be emitted to entry block of the inner most containing outlined function";
or something less verbose.

Inlined directives should emit their allocas to the entry block of the containing outined function, right? For that reason, and since we don't store an alloca insertion point in the OMPBuilder, I pass an empty insertion point, and back up the current point, without changing it.
I was planning, in the future, to keep a copy of the most recent alloca insertion point which we set when we outline, and pass that for BodyCBs within inlined directives.

3060

np.

btw, the todo note is about the CGM.Int32Ty. Currently in clang it's CGM.IntTy. I'll expand on the todo

3076

I'll do a Todo for now. I am using a very similar FiniCB to the one you used for parallel. So it doesn't make sense to change these here and not that one with them.

3099

Same as prev.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp
68

I was testing something, and I forgot to remove them when I was done.

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

Nit?

721

"The argument" refers to?

fghanim added inline comments.Jan 11 2020, 10:18 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
808

This is taken from clang (I think CG_OMP) as is. I am keeping everything as is for now in case somebody decided to help and came across it being used somewhere else, they should recognize it immediately.

So for now, I suggest a todo. cos if I find that it's not used except with critical, I'll merge all 3 functions into 1 and be done with it.

824

Same as prev.

836

same as prev.

Commits need a message that explains the change.
Also the "Summary" should not be in the commit title.

Is this something for future commits or do you want me to change these?
If the latter, how do I do that?

Already for this one, at least the git commit.
You can edit this patch in phab (top right, edit revision) and you need to make sure to adjust the git commit before pushing.

This adds support for clang to build the directives via the OMPIRBuilder so we need to add tests for that. Probably "just" running existing codegen tests for the directives with the OMPIRBuidlder enabled.

You mean unit tests? or something else? If not unit tests, Where do I do that?

We can use lit test for that. The two files that you need to modify for sure are clang/test/OpenMP/critical_codegen.cpp and clang/test/OpenMP/master_codegen.cpp. You add run lines so that the code in clang is actually executed and modify the check lines appropriately.
I did split my commits into the llvm and clang part mostly, if you look at a clang patch which uses the OpenMPIRBuilder, you should see what I mean.

As for rest of comments, anything without response, is something I will do.

I did respond to the ones with.

clang/lib/CodeGen/CGStmtOpenMP.cpp
3020

I try not to make assumptions like "Inlined directives should emit their allocas to the entry block of the containing outined function, right" if we don't need to. If we do, the assert is probably a better way to go. I would actually say we want a helper that sets/resets all these things for *all* body functions. If the OMPBuilder doesn't want to set a new allocaip, it should provide an "invalid" location to indicate that. Now I finally see what you do here and why. Go with the assert for now, and make it !isSet() if there is no reason to complicate it further.

3076

Agreed. We need a helper that is used at both/all places.

3099

Again a TODO and we are good for now.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp
68

I take it you'll remove them. Just saying because you commented ;)

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

Nit is often used to indicate a request that can be ignored silently (https://www.urbandictionary.com/define.php?term=nit)

721

Sorry. I mean passing hasFinalize instead of true

808

You can keep it a separate helper but I still don't see how it helps to keep this a member (instead of a static).

I also want the name change. My rational is simple: If we pick a name that is meaningful, people know what it does regardless of whether they know the version in clang.

824

Here, the argument that people recognize it doesn't work at all, it's a type. So please pass a StringRef if there is no other reason not to.

836

The code pattern is unrelated from "same as prev", I hope.

For the linkage, add a TODO to investigate if private linkage would work as well.

fghanim retitled this revision from Summary: Add OpenMP Directives (master and critical) to OMPBuilder, and use them in clang. to [OpenMP]{OMPIRBuilder] Add Directives (master and critical) to OMPBuilder..Jan 12 2020, 2:16 PM
fghanim edited the summary of this revision. (Show Details)
fghanim marked 4 inline comments as done.Jan 12 2020, 2:39 PM

I am working on a patch. In the mean time ;)

clang/lib/CodeGen/CGStmtOpenMP.cpp
3020

I think most (if not all) llvm passes that are concerned with allocas, expect them to be in the entry block, this is why I made this assumption. The only time that I can think of, where this will not be true is in case of a delayed outlining. In that case, the insertion block is going to be the "future" entry block of the "future" outlined function, and needs to be passed. Then I think it is also necessary to keep track of the insertion block somewhere inside the OMPBuilder.

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

before this point, the builder is always set to insert before the terminator of the finalization block. when emitInlinedFunction is done, it sets it to ExitBB.end()

824

You are right. *That* argument doesn't work. However the argument that I found it this way in clang kinda does. Also the argument that it makes things much easier to move. ;)
I assume whoever did it this way had his/her good reason. I understand you may not think it is, but given that it's used a lot in clang for getting and creating internal OMP variables, you can't really judge without going over every case to make sure that what you want can be done in every case, as such I am against the change *NOW*. However, for the time being, I included a todo basically saying that.

836

The assert is only needed if Elem.second already exist. It's quite useless if it's not and we are creating it, right?
I'll change it to an if..else, and return it once.

critical name -which is what this is being used for now- has an external linkage, isn't it?
Also, as I said this is being used by many others all over CG_OMP to create OMP internal variables.
Maybe later we'll create different versions for different internal variables, but for now I'll add a todo anyway.

fghanim updated this revision to Diff 238301.Jan 15 2020, 9:35 AM
fghanim marked 25 inline comments as done.

Addressing reviewer comments

  • Added regression tests
  • styling
  • adding asserts and todo where needed.

Also, Now EmitInlinedRegion will merge blocks where legal.

jdoerfert accepted this revision.Jan 16 2020, 1:03 AM

LGTM with some minor things you should address before the merge.

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
90

Leftover comment.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
272

Nit: typos in parameter name and comment, comments seems outdated.

305

Nit: Comment is not in sync with the Entry one.
Nit: spelling in the parameter name and format.

326

Nit: spelling param name and format.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp
45

Leftover

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

This needs to be guarded by HasFinalize if the corresponding pop is.

749

I have the feeling the merging makes it harder without providing a benefit but I'm OK with it for now

759

omp is not a good name here.

791

HasFinalize

This revision is now accepted and ready to land.Jan 16 2020, 1:03 AM
fghanim updated this revision to Diff 238591.Jan 16 2020, 1:02 PM
fghanim marked 9 inline comments as done.

Addressing reviewer's comments

  • fixed styling and naming according to llvm conventions
fghanim marked an inline comment as done and an inline comment as not done.Jan 16 2020, 1:08 PM

Thanks for reviewing this.

After I address the last two comments below, How do I merge with llvm using phab? I'd appreciate an llvm specific link if possible.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp
45

what is this referring to?

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

more difficult in what way?

Phab is not involved in the merge process, it just is a normal git push on top of the llmv-project.
Do you have commit rights already? If not I can commit this for you in a few days.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp
45

You deleted an empty new line ;)

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

We need things like auto InsertBB = merged ? ExitPredBB : ExitBB; now for example.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
686

Nit: The else is not needed.

745

Nit: The else is not needed.

762

Nit: The else is not needed.

The commit title has a stray { instead of [

fghanim retitled this revision from [OpenMP]{OMPIRBuilder] Add Directives (master and critical) to OMPBuilder. to [OpenMP][OMPIRBuilder] Add Directives (master and critical) to OMPBuilder..Jan 17 2020, 6:16 AM
fghanim updated this revision to Diff 238858.Jan 17 2020, 12:38 PM

Addressing reviewer's comments regarding styling.

No, I don't have commit privileges. I'd appreciate if you'd commit this for me. Thanks :)

I tried to push this today but the build failed:

/data/src/llvm-project/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:252:36: error: use of undeclared identifier 'BumpPtrAllocator'
  StringMap<AssertingVH<Constant>, BumpPtrAllocator> InternalVars;
                                   ^
/data/src/llvm-project/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:832:10: error: no viable conversion from returned value of type 'llvm::StringRef' to function return type 'std::string' (aka 'basic_string<char>')
  return OS.str();
         ^~~~~~~~

Can you look into that please?

Thanks for trying.

It compiles fine with me. How can I reproduce the problems you're getting? is there a way to pack what you have and send it to me so I can have a look?

Also, a second question, how trustworthy is harbormaster when it says something is buildable?

Thanks for trying.

It compiles fine with me. How can I reproduce the problems you're getting? is there a way to pack what you have and send it to me so I can have a look?

Did you rebase onto the newest llvm master?

Also, a second question, how trustworthy is harbormaster when it says something is buildable?

Honestly, I do not know.

Everything is fine!
I just cloned llvm from git hub, added this revision with arc patch. then:
cmake -G"Unix Makefiles" -DLLVM_ENABLE_PROJECTS="clang;openmp" -DCMAKE_BUILD_TYPE=Release -DLLVM_OPTIMIZED_TABLEGEN=1 -DLLVM_USE_LINKER=gold ../llvm-project/llvm
make , make check-llvm-unittest, make check-clang-openmp

all of it works fine and I got no errors whatsoever. Let me know what you think.

This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Feb 3 2020, 7:00 AM
fghanim updated this revision to Diff 242144.Feb 3 2020, 11:59 AM

Addressing errors/warnings from commit build

This revision was automatically updated to reflect the committed changes.
jdoerfert reopened this revision.Feb 10 2020, 8:35 AM
This revision is now accepted and ready to land.Feb 10 2020, 8:35 AM
fghanim updated this revision to Diff 243839.EditedFeb 11 2020, 6:00 AM

fixing unused variable.

fghanim updated this revision to Diff 244249.Feb 12 2020, 12:15 PM

Remove unnecessary preservation of builder insertion point in emitCommonDirectiveExit

This revision was automatically updated to reflect the committed changes.

I did run clang-format on the OMPIRBuilder files and replaced tabs with spaces. I think you need to teach your editor to use spaces in the future.

test/CodeGen/opt-record-1.c triggers an assertion failure.

test/CodeGen/opt-record-1.c triggers an assertion failure.

I hope the macro reordering in rGa0236de7a927 did the trick.

fhahn added a subscriber: fhahn.Feb 15 2020, 12:39 PM

It looks like this patch (or the other related changes committed recently) may cause ASan failures running Transforms/OpenMP/gtid.ll and Transforms/OpenMP/parallel_deletion.ll http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6960/consoleFull

It would be great if you could take a look and possibly revert the change if it takes some time to investigate.

It looks like this patch (or the other related changes committed recently) may cause ASan failures running Transforms/OpenMP/gtid.ll and Transforms/OpenMP/parallel_deletion.ll http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6960/consoleFull

It would be great if you could take a look and possibly revert the change if it takes some time to investigate.

Should already be fixed with rGa0236de7a927.

I wait for http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/6961/ to come back.