Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
fghanim added inline comments.Jan 11 2020, 10:18 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
813

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.

829

Same as prev.

841

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.

3075

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

3098

Again a TODO and we are good for now.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp
68 ↗(On Diff #237149)

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

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

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

726

Sorry. I mean passing hasFinalize instead of true

813

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.

829

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.

841

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
805

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()

829

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.

841

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 ↗(On Diff #238301)

Leftover comment.

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

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

306

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

327

Nit: spelling param name and format.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp
45 ↗(On Diff #238301)

Leftover

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

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

754

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

764

omp is not a good name here.

796

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 ↗(On Diff #238301)

what is this referring to?

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

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 ↗(On Diff #238301)

You deleted an empty new line ;)

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

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.