Page MenuHomePhabricator

[OpenMP] Introduce the OpenMPOpt transformation pass
Needs ReviewPublic

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.

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
19

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

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

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
19

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
145

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
19

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
145

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.Thu, Nov 14, 10:44 PM
jdoerfert added inline comments.
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
19

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
145

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.Fri, Nov 15, 7:49 AM
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
19

Coding standard

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

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.Fri, Nov 15, 9:32 AM
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
19

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.Fri, Nov 15, 9:43 AM
jdoerfert added inline comments.
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
19

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.Fri, Nov 15, 9:59 AM
llvm/include/llvm/Transforms/IPO/OpenMPOpt.h
19

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.Fri, Nov 15, 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
35

Traditionally DEBUG_TYPE itself is used for this.

116

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

Value *GTIdArg = find_if() be better?

214–215

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.