This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Introduce the OpenMPOpt transformation pass
ClosedPublic

Authored by jdoerfert on Nov 6 2019, 9:31 PM.

Details

Summary

The OpenMPOpt pass is a CG-SCC pass in which OpenMP specific
optimizations can reside.

The OpenMPOpt pass uses the OpenMPKinds.def file to identify runtime
calls and their uses. This allows targeted transformations and eases
their implementation.

This initial patch deduplicates __kmpc_global_thread_num calls. We can
also identify arguments that are equivalent to such a call result and
use it instead. Later we can determine "gtid" arguments based on the use
in kernel functions etc.

Diff Detail

Event Timeline

jdoerfert created this revision.Nov 6 2019, 9:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2019, 9:31 PM
jdoerfert updated this revision to Diff 228177.Nov 6 2019, 9:34 PM

Remove leftover change

Harbormaster completed remote builds in B40606: Diff 228177.
jdoerfert updated this revision to Diff 228266.Nov 7 2019, 9:38 AM

Rebase after types moved to a central location

No old-pm?

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
49

IsVarArg

84

reusing

105

reusing

jdoerfert updated this revision to Diff 228359.Nov 7 2019, 9:11 PM
jdoerfert marked 3 inline comments as done.

Add old-PM support and fix typos

jdoerfert updated this revision to Diff 228362.Nov 7 2019, 9:22 PM

Make sure to remove cached uses when calls are removed

ABataev added inline comments.Nov 8 2019, 7:31 AM
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
20

Better to use class? Also, maybe worth it to mark it as final.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
146

What if the function is already in the entry block?

jdoerfert marked 2 inline comments as done.Nov 8 2019, 10:03 AM
jdoerfert added inline comments.
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
20

I can do that but I do not understand why this would be better (in a lot of situations, including this one).
After all, class will just cause an additional public and that is it (for all but MSVC and only if we declare this struct/class again which we never do). Similarly, I have never seen an attempt to inherit from a new-PM style pass and if so it is unclear why we should forbid it (with final).

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
146

Not a problem and not sufficient:

If we have the following entry and the first use we visit is (for some reason) %gtid1 we need to move it because it would not dominate the use of %gtid0.

entry:
  %gtid0 = call __kmpc_global_thread_num ...
  ... use(%gtid0)
  %gtid1 = call __kmpc_global_thread_num ...
ABataev added inline comments.Nov 8 2019, 10:31 AM
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
20

Up to you. But if you going to have private or protected members later, better to make class rather than struct.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
146

The question is a little bit different. What if we have something simple:

entry:
  %gtid0 = call __kmpc_global_thread_num ...
  ... use(%gtid0)

Will this pass try to move the call after the use? I'm not saying that this will definitely happen, I'm just asking if pass of aware of such situation and will handle it properly.

jdoerfert marked 4 inline comments as done.Nov 14 2019, 10:44 PM
jdoerfert added inline comments.
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
20

Up to you. But if you going to have private or protected members later, better to make class rather than struct.

Why? They are the same except the default visibility.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
146

This moves the call unconditionally to the very first position in a function. That is by definition before all uses. We can add smarts wrt. the placement later.

ABataev added inline comments.Nov 15 2019, 7:49 AM
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
20

Coding standard

jdoerfert marked 5 inline comments as done.Nov 15 2019, 9:25 AM
jdoerfert added inline comments.
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
20

The Coding standard states, as a rule of thumb, that the right choice here is a struct:

https://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords

ABataev added inline comments.Nov 15 2019, 9:32 AM
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
20

But if you going to have private or protected members later, better to make class rather than struct.

jdoerfert marked 3 inline comments as done.Nov 15 2019, 9:43 AM
jdoerfert added inline comments.
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
20

The important point is, this does not have a private or protected member now.

There are two things to my argument:

  1. We do not design for what might or might not happen as that regularly results in bad design decisions. Assuming one would later change something, e.g., add a protected member, once could change the struct to a class.
  2. It is unrealistic to assume a PassInfoMixin object like this one to ever have more members. This is just an interface for the pass manager to interact with the pass, and while there are exceptions, most of these interfaces I've seen only contain a run method.
ABataev added inline comments.Nov 15 2019, 9:59 AM
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
20

Just like I said, it is up to you. It was just a suggestion to avoid future possible modifications.

jdoerfert marked 2 inline comments as done.Nov 15 2019, 10:04 AM

Anything else?

I'm really not the best person to review this :/
It seems generally within what'd expect, but i'm not really familiar with SCC passes.
Some thoughts:

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
36

Traditionally DEBUG_TYPE itself is used for this.

117

I'm guessing this is 'llvm::for_each() but until first match'?
Maybe something like

Value *GTIdArg = find_if() be better?

215–216

Here and elsewhere: i'm guessing AddUserArgs() will update GTIdArgs so it must not be range-based for and we can't precache GTIdArgs.size(). This should be commented.

I'm really not the best person to review this :/

No one (I ask) seems to be. At some point, I argue, it should be enough that it looks good to multiple "non-experts". We can always revert it.
Though, I will make it easier, see below.

It seems generally within what'd expect, but i'm not really familiar with SCC passes.

I will add a call graph helper soon, given that I think I figured out how to use both of them.
I hope @chandlerc or someone else familiar with both call graphs (for old & new pass manager) will review them.
Assuming that works, there is little SCC related left that is not "provided" by this helper.

Some thoughts:

I'll adjust according to your comments shortly.

Also, this is missing some statistics, and maybe some debug counters.

The plumbing looks sane to me. I imagine any bugs or limitations we miss at review will make themselves apparent as more complicated optimisations are included.

@ABataev do you have remaining concerns with the current patch?

sstefan1 resigned from this revision.Jan 13 2020, 9:27 PM

Also, this is missing some statistics, and maybe some debug counters.

I'll rebase soon and add them.

jdoerfert updated this revision to Diff 241972.Feb 2 2020, 10:40 PM
jdoerfert marked 3 inline comments as done and an inline comment as not done.

Use D70927, address comments, introduce some statistic counters

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jdoerfert marked an inline comment as done.Feb 3 2020, 3:13 PM

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jdoerfert updated this revision to Diff 242228.Feb 3 2020, 4:07 PM

Explicitly check for OpenMP runtime calls and stand down if none found.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jdoerfert updated this revision to Diff 242256.Feb 3 2020, 10:11 PM

Replace llvm::find_if with an explicit loop that works

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

lebedev.ri added a comment.EditedFeb 4 2020, 7:47 AM

Some nits.
I think this should proceed, the intersection between "llvm developers"
and "openmp developers" appears to be only forming, and it can't possibly ever form
if the patches that form it are stuck waiting for the intersection to form :)

llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
32

enum struct

llvm/lib/Passes/PassBuilder.cpp
841–843

Personally, i'd think this should only be run in -O2 and higher (i.e. not under -O0/-O1)

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
145–146

Should there be a debug counter controlled early exit as the first line of function?

jdoerfert updated this revision to Diff 242914.Feb 6 2020, 8:13 AM
jdoerfert marked 4 inline comments as done.

Addressed comments

JonChesterfield accepted this revision.Feb 6 2020, 8:34 AM

Still looks good to me. I'll even mark it as such.

Agreed with the above sentiment - we need the infra to build on in order to progress.

This revision is now accepted and ready to land.Feb 6 2020, 8:34 AM
This revision was automatically updated to reflect the committed changes.
wenlei added a subscriber: wenlei.Feb 24 2020, 9:28 PM
wenlei added inline comments.
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
160

I think you need to make sure it's not moved before the def of the operand for openmp calls.

I'm seeing a case where __kmpc_global_thread_num has a parameter, and the call is moved to before def of parameter, see snippet below.

%5           = call i32 @__kmpc_global_thread_num(%struct.ident_t* nonnull %6)
%6           = alloca %struct.ident_t, align 8

This line needs to be guarded with #if !defined(NDEBUG) to prevent warning-as-error failures in release build.

static constexpr auto TAG = "[" DEBUG_TYPE "]";

The change made in https://reviews.llvm.org/rGa50c0b0df733423f9f6f92bb4e0be26f73326ae3 is not sufficient to prevent release build issue. Could you fix this? Thanks!

This line needs to be guarded with #if !defined(NDEBUG) to prevent warning-as-error failures in release build.

static constexpr auto TAG = "[" DEBUG_TYPE "]";

The change made in https://reviews.llvm.org/rGa50c0b0df733423f9f6f92bb4e0be26f73326ae3 is not sufficient to prevent release build issue. Could you fix this? Thanks!

I don't understand why this is not sufficient. Could you explain that please? (I usually test with release+asserts)

llvm/lib/Passes/PassBuilder.cpp
841–843

I can do that.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
145–146

I don't understand what you want here, could you explain?

This line needs to be guarded with #if !defined(NDEBUG) to prevent warning-as-error failures in release build.

static constexpr auto TAG = "[" DEBUG_TYPE "]";

The change made in https://reviews.llvm.org/rGa50c0b0df733423f9f6f92bb4e0be26f73326ae3 is not sufficient to prevent release build issue. Could you fix this? Thanks!

I don't understand why this is not sufficient. Could you explain that please? (I usually test with release+asserts

! In D69930#1901384, @jdoerfert wrote:

! In D69930#1900147, @xiaoqing_wu wrote:

This line needs to be guarded with #if !defined(NDEBUG) to prevent warning-as-error failures in release build.

static constexpr auto TAG = "[" DEBUG_TYPE "]";

The change made in https://reviews.llvm.org/rGa50c0b0df733423f9f6f92bb4e0be26f73326ae3 is not sufficient to prevent release build issue. Could you fix this? Thanks!

I don't understand why this is not sufficient. Could you explain that please? (I usually test with release+asserts)

TAG is only used when NDEBUG is not defined. It's not used when NDEBUG is not defined and LLVM_ENABLE_DUMP is enabled. To reproduce this, run cmake with these options: -DLLVM_ENABLE_DUMP=On -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=Off.

This line needs to be guarded with #if !defined(NDEBUG) to prevent warning-as-error failures in release build.

static constexpr auto TAG = "[" DEBUG_TYPE "]";

The change made in https://reviews.llvm.org/rGa50c0b0df733423f9f6f92bb4e0be26f73326ae3 is not sufficient to prevent release build issue. Could you fix this? Thanks!

I don't understand why this is not sufficient. Could you explain that please? (I usually test with release+asserts

! In D69930#1901384, @jdoerfert wrote:

! In D69930#1900147, @xiaoqing_wu wrote:

This line needs to be guarded with #if !defined(NDEBUG) to prevent warning-as-error failures in release build.

static constexpr auto TAG = "[" DEBUG_TYPE "]";

The change made in https://reviews.llvm.org/rGa50c0b0df733423f9f6f92bb4e0be26f73326ae3 is not sufficient to prevent release build issue. Could you fix this? Thanks!

I don't understand why this is not sufficient. Could you explain that please? (I usually test with release+asserts)

TAG is only used when NDEBUG is not defined. It's not used when NDEBUG is not defined and LLVM_ENABLE_DUMP is enabled. To reproduce this, run cmake with these options: -DLLVM_ENABLE_DUMP=On -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=Off.

Sorry for the wait, I got side-tracked bad. Should be fixed by D75970.