Page MenuHomePhabricator

[OpenMP 5.0] Codegen support to pass user-defined mapper functions to runtime
Needs ReviewPublic

Authored by lildmh on Sep 20 2019, 6:51 AM.

Details

Summary

This patch implements the code generation to use OpenMP 5.0 declare mapper (e.g., user-defined mapper) constructs. It looks up the proper mapper function for each map, to, or from clause that has a user-defined mapper associated, and passes them to the OpenMP runtime function.
The design slides can be found at https://github.com/lingda-li/public-sharing/blob/master/mapper_runtime_design.pptx

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
lildmh updated this revision to Diff 221578.Sep 24 2019, 1:00 PM
lildmh added a project: Restricted Project.

I would suggest to split this patch into 2 part: the first one is NFC and just makes the ompiler to use the new interfaces and the second with the new functionality for mappers.

include/clang/AST/OpenMPClause.h
4292

Not in Camel format

4537–4541

Why changed to const Expr * here? I believe, ArrayRef makes it constant automatically.

4590

Not in Camel format

4593

No need for const in ArrayRef

4615–4616

Not in Camel format

4737

ArrayRef<const Expr *>()->llvm::None?

4744

ArrayRef<const Expr *>()->llvm::None

4757

Same here

lib/CodeGen/CGOpenMPRuntime.cpp
742

Better to fix it in a separate patch

755–794

I believe these new functions replace completely the old one, so old functions must be removed

2687

Same here, remove processing for old functions.

7456

CamelCase

7456

Seems to me, with mapper you're changing the contract of this function. It is supposed to return the size in bytes, with Mapper it returns just number of elements. Bad decision. Better to convert size in bytes to number of elements in place where you need it.

7477

Why do we return 1 here?

7478

No need for else here

7497–7500

Same here

7509

No need for else here

7513–7522

Same here

7608–7617

Too many params in the function, worth thinking about wrapping them in a record.

7984–7987

Mappers.push_back(HasMappers?Mapper:nullptr);

8760–8762

const auto *VD = dyn_cast_or_null<VarDecl>(std::get<0>(L));

8865–8866

In general, there should not be branch for default mapping during the codegen, the implicit map clauses must be generated in sema.

8997–8999

Better to use Builder.CreateConstArrayGEP

9000–9001

Easier cast function to the voidptr, I think

9035–9038

Hmm, just cast of the Info.MappersArray to the correct type if all indices are 0?

9039–9040

If then sub-statement in braces, the else substatement also must be in braces

9400

Better to use MapperCGF.EmitNounwindRuntimeCall.

lib/CodeGen/CGOpenMPRuntime.h
1525

Camel case

1541

The flag HasMappers also must be cleared, no?

lildmh updated this revision to Diff 234113.Dec 16 2019, 11:39 AM
lildmh marked 28 inline comments as done.

Rebase and address comments

I'll split the patch into 2 later

lib/CodeGen/CGOpenMPRuntime.cpp
7456

Yes. The reason why I did this is I found this is the single place that I can consider all situations. Otherwise, I need to divide the size by the element size in other places. Since this function has discussed all situations for the element size, I will need to duplicate the work elsewhere if I don't do it here. The latter solution is not good for code mountainous I think.

7608–7617

Maybe not in this patch?

8865–8866

Not sure where this code will be used. I guess it's still correct to add a null to the mappers here?

8997–8999

I saw all the other places are using CreateConstInBoundsGEP2_32 to get sizes, types, etc. So I kept it consistent here. Maybe these modifications should be in a future patch?

9000–9001

Again, this is to keep it consistent with previous code above?

9035–9038

Again, this is to keep consistent with previous code?

ABataev added inline comments.Dec 16 2019, 12:14 PM
clang/include/clang/AST/OpenMPClause.h
4918 ↗(On Diff #234113)

Do we really need to set HasMapper to true unconditionally here and in other places?

clang/lib/CodeGen/CGOpenMPRuntime.h
841–842 ↗(On Diff #234113)

Is this required to make it virtual?

1556 ↗(On Diff #234113)

(!HasMapper || MappersArray)?

lib/CodeGen/CGOpenMPRuntime.cpp
7456

Changing the original function contract is also not the best solution. Better to introduce the new function. If some of the code can be reused, need to factor out the common code and reuse it in the old function and the new one.

7456

Also, additional question. Does it mean that we pass to the runtime functions different values in different situations: in one case we pass size in bytes, with mappers - number of elements? If so, it is a very bad idea caused by bad design. We need to pass either size in bytes, or number of elements in this argument.

7608–7617

It is better to make it in this patch or make a separate patch and commit it before this one. We try to resolve this as soon as possible.

8865–8866

I mean, that in general, it would be good to see here some kind of an assertion. Almost all situations must be resolved in Sema.

8997–8999

Some of the modifications were made before we had these new functions. Now, we have the new ones and if we can use it, better to switch to the new safer versions.

9000–9001

No need to rely on the old code if can make something cleaner and better. Cast Mfunc to voidptr in the if statement and no need to make some extra casts later.

9035–9038

Same, use the best code if we can use it.

lildmh marked 5 inline comments as done.Dec 16 2019, 12:41 PM

Alexey, thanks for the review

clang/include/clang/AST/OpenMPClause.h
4918 ↗(On Diff #234113)

For map, to, and from clauses, they can have mappers, so it's set to true. For is_device_ptr and use_device_ptr, it's set to false.

lib/CodeGen/CGOpenMPRuntime.cpp
7456

Yes, the current design is like this. Let me see how to make it better

8865–8866

Okay, I guess it's not the work of this patch

ABataev added inline comments.Dec 16 2019, 12:45 PM
clang/include/clang/AST/OpenMPClause.h
4918 ↗(On Diff #234113)

So, it completely depends on the clause kind, right? If so, can we just remove it and rely on the clause kind?

lildmh marked 4 inline comments as done.Dec 16 2019, 4:38 PM
lildmh added inline comments.
clang/include/clang/AST/OpenMPClause.h
4918 ↗(On Diff #234113)

This is used in OMPMappableExprListClause which is inherited by all these clauses. Do you mean to check the template type there to get the clause kind?

ABataev added inline comments.Dec 17 2019, 7:31 AM
clang/include/clang/AST/OpenMPClause.h
4918 ↗(On Diff #234113)

I see. I think it is better to rename it to ClauseSupportsMapper (or something like this) and make it const. Currently, it is confusing. I thought that this flag shows the use of the mapper in the clause.

lildmh marked an inline comment as done.Dec 17 2019, 7:46 AM
lildmh added inline comments.
clang/include/clang/AST/OpenMPClause.h
4918 ↗(On Diff #234113)

Ok, will do that

lildmh updated this revision to Diff 234554.Dec 18 2019, 8:53 AM
lildmh marked 3 inline comments as done.

Address Alexey's comments to change mapper function size and refactor code

Looks much better in general, need to resolve last one issue with number of elements/size and you're ready.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
8981–8982 ↗(On Diff #234554)

So, we're still going to use number of elements for mappers? And pass it in the same parameter that in other cases is used as size in bytes? If so, point to it explicitly in the review for the runtime part so all are informed about it.

9244 ↗(On Diff #234554)

getOrEmitUserDefinedMapperFunc?

lildmh marked 2 inline comments as done.Dec 18 2019, 9:40 AM
lildmh added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8981–8982 ↗(On Diff #234554)

From interface, the mapper function uses size in bytes now. Inside, it needs number of elements to iterate through all elements. This has no impact on the runtime part, since it looks like normal mapping from the interface. All conversion happens inside the mapper function which is completely generated by the compiler.

9244 ↗(On Diff #234554)

I guess getUserDefinedMapperFunc is a better name? Because the user uses this function to get the mapper function. Emitting a mapper function is a side effect.

ABataev added inline comments.Dec 18 2019, 9:43 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8981–8982 ↗(On Diff #234554)

Ok. Then why do we need to convert size in bytes to number of elements here?

9244 ↗(On Diff #234554)

I meant something like getOrCreate... but it is up to you.

lildmh marked 2 inline comments as done.Dec 18 2019, 9:54 AM
lildmh added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8981–8982 ↗(On Diff #234554)

This is used to 1) see if we are going to map an array of elements with mapper, and 2) iterate all to map them individually.

9244 ↗(On Diff #234554)

Okay, sounds good

ABataev added inline comments.Dec 18 2019, 10:00 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8981–8982 ↗(On Diff #234554)

Could you point where we have this kind of analysis here? Because I don't see anything affected by this change in the patch.

lildmh marked 2 inline comments as done.Dec 18 2019, 10:31 AM
lildmh added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8986 ↗(On Diff #234554)

Size is used to compute the last element position, which is used as the loop boundary.

9207 ↗(On Diff #234554)

Size is used to see if this is an array.

ABataev added inline comments.Dec 18 2019, 10:40 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8981–8982 ↗(On Diff #234554)

Is this a bug fix in the previous implementation?

lildmh updated this revision to Diff 234579.Dec 18 2019, 10:44 AM

Change the function name

lildmh marked an inline comment as done.Dec 18 2019, 10:54 AM
lildmh added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8981–8982 ↗(On Diff #234554)

The previous implementation assumes the size is the number of elements, and it works correctly under that assumption. Since we change the meaning of size here, I add this line of code so the previous implementation can work correctly in the new assumption that the size is the size in bytes.

ABataev added inline comments.Dec 18 2019, 10:59 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8981–8982 ↗(On Diff #234554)

Ah, got it. Then, in general, looks good. Please, split the patches in to, possibly, 2 NFC (one with using of the new functions + another one for aggregating too many params into records) + another one with the new functionality.

lildmh marked an inline comment as done.Dec 18 2019, 11:14 AM
lildmh added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8981–8982 ↗(On Diff #234554)

Okay, will do that. What do you think need to be done for the runtime patch D68100?

ABataev added inline comments.Dec 18 2019, 11:18 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8981–8982 ↗(On Diff #234554)

Address comments and rebase, I think

lildmh marked an inline comment as done.Dec 18 2019, 11:29 AM
lildmh added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8981–8982 ↗(On Diff #234554)

The only left comment is to split it into 2 patches, I think.

ABataev added inline comments.Dec 18 2019, 11:36 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
8981–8982 ↗(On Diff #234554)

Do it.