This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Add support for registering requires directives with the runtime
ClosedPublic

Authored by gtbercea on Apr 11 2019, 8:48 AM.

Details

Summary

This patch adds support for the registration of the requires directives with the runtime.

Each requires directive clause will enable a particular flag to be set.

The set of flags is passed to the runtime to be checked for compatibility with other such flags coming from other object files.

The registration function is called whenever OpenMP is present even if a requires directive is not present. This helps detect cases in which requires directives are used inconsistently.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2019, 8:48 AM
ABataev added inline comments.Apr 11 2019, 9:20 AM
lib/CodeGen/CGDecl.cpp
2576 ↗(On Diff #194700)

You don't need to pass the reference to CodeGenModule to CGOpenMPRuntime class, it handles a reference to the CodeGenMode already.

lib/CodeGen/CGOpenMPRuntime.cpp
726 ↗(On Diff #194700)

You must use LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE(); and LLVM_MARK_AS_BITMASK_ENUM(/* LargestFlag = */) from clang/Basic/BitmaskEnum.h to enable bit operations on your new bit-arithmetic based enumeric.

726 ↗(On Diff #194700)

Add a comment for the whole new type

728 ↗(On Diff #194700)

You should not handle all the possible flags for the requires directives, since you try to support only target-specific flags.

2322 ↗(On Diff #194700)

Function return type must be void

3870 ↗(On Diff #194700)

Why do you need a new member function? Can you make a static local function?

3882 ↗(On Diff #194700)

Use getTypes().arrangeNullaryFunction()

3887 ↗(On Diff #194700)

Use OpenMPOffloadingRequiresDirFlags instead of int64_t

7983 ↗(On Diff #194700)

Seems to me, this must be in another patch, has nothing to do with this patch

9069 ↗(On Diff #194700)

Why do you need the second function?

lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
4946 ↗(On Diff #194700)

Call this function after the target-specific processing.

lib/CodeGen/CodeGenModule.h
294 ↗(On Diff #194700)

Why public? Must be private. Also, add comments for this new data member.
Also, I don't think this must be in CGM. It must be a member of CGOpenMPRuntime class.

gtbercea updated this revision to Diff 194741.Apr 11 2019, 1:34 PM
  • Address comments.
gtbercea updated this revision to Diff 194743.Apr 11 2019, 1:42 PM
gtbercea marked 6 inline comments as done.
  • Address comments.
gtbercea marked 4 inline comments as done.Apr 11 2019, 1:46 PM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.cpp
3870 ↗(On Diff #194700)

Same as for register lib.

3887 ↗(On Diff #194700)

Leads to error if I do that. This enum behaves like OpenMPLocationFlags.

7983 ↗(On Diff #194700)

I will split it.

9069 ↗(On Diff #194700)

Second function eliminated.

gtbercea updated this revision to Diff 194758.Apr 11 2019, 2:14 PM
gtbercea marked 2 inline comments as done.
  • Fix enum.
gtbercea marked 2 inline comments as done.Apr 11 2019, 2:15 PM
gtbercea updated this revision to Diff 194760.Apr 11 2019, 2:22 PM
  • Split patch.
gtbercea updated this revision to Diff 194781.Apr 11 2019, 4:09 PM
  • Handle OpenMP simd case.
AlexEichenberger requested changes to this revision.Apr 12 2019, 6:16 AM

see note above; apologies if it is already done and hiding somewhere I did not see

lib/CodeGen/CGOpenMPRuntime.h
641 ↗(On Diff #194781)

Don't you need a bool for each characteristics? Your intention is to have one bit vector for each characteristics that matter to the compiler?

Also, it is my belief that you need to record 2 states:
unmaterialized (meaning I have not processed any target yet, so I should record any requires as they arrive)
finalized (I am processing a target, so the state is now fixed)

you need this in case you have an input like this:

declare target
int x
end declare target

requires unified memory

which is illegal

This revision now requires changes to proceed.Apr 12 2019, 6:16 AM
ABataev added inline comments.Apr 12 2019, 7:53 AM
lib/CodeGen/CGOpenMPRuntime.cpp
473 ↗(On Diff #194781)

YOu don't need al these flags, add only target-specific.

1254 ↗(On Diff #194781)

No need to do this initialization here

8978 ↗(On Diff #194781)

Just HasRequiresUnifiedSharedMemory = true;

9045 ↗(On Diff #194781)

Just add a check for OpenMPSimd mode here and do not emit anything in this case.

9061 ↗(On Diff #194781)

No need for braces

9065 ↗(On Diff #194781)

CGF.CGM.Int64Ty->CGM.Int64Ty

lib/CodeGen/CGOpenMPRuntime.h
1438 ↗(On Diff #194781)

Why do you need this vertual funtion? I think static local is going to be enough

lib/CodeGen/CodeGenModule.cpp
415 ↗(On Diff #194781)

You can remove the third nullptr argument

gtbercea updated this revision to Diff 195185.Apr 15 2019, 8:03 AM
  • Address comments.
gtbercea marked 7 inline comments as done.Apr 15 2019, 8:05 AM
gtbercea updated this revision to Diff 195186.Apr 15 2019, 8:11 AM
  • Remove atomic flags.
gtbercea marked an inline comment as done.Apr 15 2019, 8:13 AM
ABataev added inline comments.Apr 15 2019, 8:23 AM
lib/CodeGen/CGOpenMPRuntime.cpp
8970 ↗(On Diff #195186)

You can do break; here, no need for further analysis

9050 ↗(On Diff #195186)

Change the type from int64_t to OpenMPOffloadingRequiresDirFlags

8978 ↗(On Diff #194781)

Not done

lib/CodeGen/CGOpenMPRuntime.h
641 ↗(On Diff #194781)

What about this comment?

1438 ↗(On Diff #194781)

Can you make it const member function?

gtbercea updated this revision to Diff 195190.Apr 15 2019, 8:28 AM
  • Fix type.
gtbercea marked an inline comment as done.Apr 15 2019, 8:28 AM
gtbercea marked 2 inline comments as done.Apr 15 2019, 8:38 AM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.cpp
8978 ↗(On Diff #194781)

Ah I misunderstood your initial comment. I thought you were complaining about the break being in there.

8978 ↗(On Diff #194781)

Because the member is non-static I have to provide the CGM.getOpenMPRuntime() part.

gtbercea updated this revision to Diff 195195.Apr 15 2019, 8:41 AM
  • Add break.
gtbercea marked an inline comment as done.Apr 15 2019, 8:41 AM
gtbercea marked 2 inline comments as done.Apr 15 2019, 8:44 AM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.h
1438 ↗(On Diff #194781)

Cannot add that due to createRuntimeFunction not being const.

gtbercea marked 3 inline comments as done.Apr 15 2019, 8:45 AM
gtbercea updated this revision to Diff 195200.Apr 15 2019, 8:58 AM
  • Remove const.
ABataev added inline comments.Apr 15 2019, 9:04 AM
lib/CodeGen/CGOpenMPRuntime.cpp
8978 ↗(On Diff #194781)

Because the member function is non-static and the member data is non-static, you can just use HasRequiresUnifiedSharedMemory = true;

test/OpenMP/openmp_offload_registration.cpp
38 ↗(On Diff #195200)

Add a test where the unified memory flag is used

gtbercea marked an inline comment as done.Apr 15 2019, 9:10 AM
gtbercea marked an inline comment as done.
gtbercea added inline comments.
test/OpenMP/openmp_offload_registration.cpp
38 ↗(On Diff #195200)

Agreed. More test will be coming anyway. There are a lot of regression tests which need to be updated.

gtbercea updated this revision to Diff 195394.Apr 16 2019, 8:48 AM
  • Fix checks for use of requires directive.
gtbercea marked an inline comment as done.Apr 16 2019, 8:49 AM
ABataev added inline comments.Apr 16 2019, 8:56 AM
lib/CodeGen/CGOpenMPRuntime.h
644 ↗(On Diff #195394)

Why do you need this? I think your function should be called without any conditions. It does not depend on the presence of the target regions or not. Plus, your check is not consistent if you're calling the function from another module, which has target region inside.

gtbercea marked an inline comment as done.Apr 16 2019, 9:01 AM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.h
644 ↗(On Diff #195394)

This does not determine if the function is called or not. This helps determine the flags with which the function is called.

ABataev added inline comments.Apr 16 2019, 9:06 AM
lib/CodeGen/CGOpenMPRuntime.h
644 ↗(On Diff #195394)

It does not matter, it still does not work correctly if the target region is called in the function from another module

gtbercea marked an inline comment as done.Apr 16 2019, 10:18 AM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.h
644 ↗(On Diff #195394)

If the target is in another compilation unit, that unit will need to have a requires directive.

gtbercea marked an inline comment as done.Apr 16 2019, 10:21 AM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.h
644 ↗(On Diff #195394)

If you can come up with an example which you think doesn't work I'm happy to try it.

ABataev added inline comments.Apr 16 2019, 11:22 AM
lib/CodeGen/CGOpenMPRuntime.h
644 ↗(On Diff #195394)

If the module without target directives was compiled with unified memory and the module with target directives compiled without unfied memory support? Is this a problem? Shall we diagnose it?

gtbercea marked an inline comment as done.Apr 16 2019, 11:48 AM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.h
644 ↗(On Diff #195394)

The requires directives in a module without targets are just not taken into consideration. In general, a target region is needed before the requires flags are checked for compatibility with flags in other modules.

ABataev added inline comments.Apr 16 2019, 11:51 AM
lib/CodeGen/CGOpenMPRuntime.h
644 ↗(On Diff #195394)

You're saying that we're going to ignore the directive completely if the module does not have target regions. I'd suggest to discuss it with Alex if this is ok per standard.

This revision is now accepted and ready to land.Apr 16 2019, 12:16 PM
gtbercea marked an inline comment as done.Apr 16 2019, 12:36 PM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.h
644 ↗(On Diff #195394)

I discussed with Alex and we agreed on this being the most practical solution. This will enable users to ensure consistency of requires flags across relevant compilation units only.

ABataev added inline comments.Apr 16 2019, 12:37 PM
lib/CodeGen/CGOpenMPRuntime.cpp
8975 ↗(On Diff #195394)

The message speaks about target region but points to the clause. You need to correct the message.

9065 ↗(On Diff #195394)

Hmm, according to the standard A requires directive with any of the following clauses must appear in all compilation units of a program that contain device constructs or device routines or in none of them. You can not detect use of the device routines in the code, can you?

lib/CodeGen/CGOpenMPRuntime.h
644 ↗(On Diff #195394)

Must start from capital letter

gtbercea updated this revision to Diff 195608.Apr 17 2019, 11:32 AM
gtbercea marked an inline comment as done.
  • Add support for declare target routines.
gtbercea updated this revision to Diff 195609.Apr 17 2019, 11:41 AM
gtbercea marked an inline comment as done.
  • Update error message.
gtbercea marked 2 inline comments as done.Apr 17 2019, 11:42 AM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.cpp
8975 ↗(On Diff #195394)

"A target region was emitted before this requires directive." ?

gtbercea marked an inline comment as done.Apr 17 2019, 11:43 AM

Other tests in presence of the requires directive with the clause?

lib/CodeGen/CGOpenMPRuntime.cpp
7956 ↗(On Diff #195608)

Restore the original, this must be changed in a separate patch.

8975 ↗(On Diff #195394)

Still does not look good, better to add previously emitted target regions in the diagnostic somehow. Also, it would be good to mention the clause in the message.

test/OpenMP/openmp_offload_registration.cpp
40 ↗(On Diff #195608)

Why do you need this check?

gtbercea updated this revision to Diff 195644.Apr 17 2019, 3:29 PM
  • Update tests.
  • Move error check in sema.
ABataev added inline comments.Apr 18 2019, 6:51 AM
lib/CodeGen/CGOpenMPRuntime.cpp
7956 ↗(On Diff #195608)

Restore

lib/Sema/SemaOpenMP.cpp
196 ↗(On Diff #195644)

I would suggest to split this patch into 2 parts: the sema checking part and the codegen part.

468 ↗(On Diff #195644)

Make function const

469 ↗(On Diff #195644)

const OMPClause *

476 ↗(On Diff #195644)

DSAStack usually is not used for diagnostic emission, it serves as the container. Better to move the diagnostics to Act... functions.

gtbercea marked an inline comment as done.Apr 18 2019, 7:32 AM
gtbercea added inline comments.
lib/Sema/SemaOpenMP.cpp
476 ↗(On Diff #195644)

The "bool hasDuplicateRequiresClause(ArrayRef<OMPClause *> ClauseList) const {" function works in a similar function as this one and does emit diagnostics. See a few lines above.

I need to iterate through a private member of DSAStack so I don't see how that can be in any other place than in DSAStack.

ABataev added inline comments.Apr 18 2019, 7:33 AM
lib/Sema/SemaOpenMP.cpp
476 ↗(On Diff #195644)
  1. hasDuplicateRequiresClause must be fixed.
  2. You can return ArrayRef<SourceLocation> and iterate over it in the Act.. function
gtbercea updated this revision to Diff 195739.Apr 18 2019, 7:52 AM
  • Add const.
  • Address comments.
gtbercea marked 4 inline comments as done.Apr 18 2019, 7:53 AM
ABataev added inline comments.Apr 18 2019, 7:58 AM
include/clang/Basic/DiagnosticSemaKinds.td
9135 ↗(On Diff #195739)

Split the patch, sema part must be separate.

gtbercea updated this revision to Diff 195775.Apr 18 2019, 10:18 AM
  • Move error check in sema.
gtbercea marked an inline comment as done.Apr 18 2019, 10:19 AM
ABataev added inline comments.Apr 18 2019, 12:38 PM
test/OpenMP/openmp_offload_registration.cpp
40 ↗(On Diff #195608)

Again, this check is not required

test/OpenMP/target_codegen.cpp
99 ↗(On Diff #195775)

Add a check for function too

gtbercea updated this revision to Diff 196945.Apr 26 2019, 7:51 PM
gtbercea marked 2 inline comments as done.
  • Add tests.
gtbercea marked an inline comment as done.Apr 26 2019, 7:52 PM
ABataev added inline comments.Apr 29 2019, 7:26 AM
lib/CodeGen/CGOpenMPRuntime.cpp
10362 ↗(On Diff #196945)

Bad formatting.

10364 ↗(On Diff #196945)

No need for the braces

10364 ↗(On Diff #196945)

What if declare target is used only for variabes but not for the functions?

gtbercea marked an inline comment as done.May 5 2019, 12:10 PM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.cpp
10364 ↗(On Diff #196945)

Even more reason to error in that case since it may contain clauses like link or to which need for requires directives to be used consistently.

ABataev added inline comments.May 6 2019, 7:17 AM
lib/CodeGen/CGOpenMPRuntime.cpp
10364 ↗(On Diff #196945)

But I don't see that your patch works in this situation. Currently, it will emit the error only if the declare target function is found, no?

gtbercea marked an inline comment as done.May 17 2019, 8:02 AM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.cpp
10364 ↗(On Diff #196945)

Actually it will work even when just variables are used in the declare target region. There is another problem which I have a fix for. I will update the patch in a bit.

ABataev added inline comments.May 17 2019, 8:10 AM
lib/CodeGen/CGOpenMPRuntime.cpp
10364 ↗(On Diff #196945)

Why will it work in this case? If you have just a declare target region in the code for the variables and nothing else. You don't have target regions, declare target functions etc. It won't work in this case.

gtbercea marked an inline comment as done.May 17 2019, 8:22 AM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.cpp
10364 ↗(On Diff #196945)

It will work because the OffloadEntriesInfoManager.empty() will return false in that case.

ABataev added inline comments.May 17 2019, 8:55 AM
lib/CodeGen/CGOpenMPRuntime.cpp
9291 ↗(On Diff #196945)

Missed check for HasEmittedTargetRegion here

9312 ↗(On Diff #196945)

The first part of the condition is excessive. You have early exit from the function. Instead of the condition, use assert((!OffloadEntriesInfoManager.empty() || HasEmittedDeclareTargetRegion || HasEmittedTargetRegion) && "Expected bla-bla-bla");

10364 ↗(On Diff #196945)

But you don't have a check for OffloadEntriesInfoManager.empty() when you set Flags for __tgt_register_requires function call.

gtbercea updated this revision to Diff 200088.May 17 2019, 1:31 PM
  • Fix conditions.
gtbercea updated this revision to Diff 200089.May 17 2019, 1:34 PM
gtbercea marked 3 inline comments as done.
  • Fix format.
gtbercea updated this revision to Diff 200091.May 17 2019, 1:36 PM
  • Fix braces.
gtbercea marked 5 inline comments as done.May 17 2019, 1:37 PM

Looks good, in general. Just one more question: will tgt_register_requires be emitted if only -fopenmp is used? If -fopenmp-targets is not provided, this function should not be called, I think, because we're not going to use offloading at all in this case and tgt_register_requires should not be emitted. Otherwise, we may end up with the linker error that tgt_register_requires is not found. Would be good to have a test with -fopenmp and #pragma omp requires that check that no tgt_register_requires is emitted in this case.

gtbercea updated this revision to Diff 200355.May 20 2019, 2:06 PM
  • Avoid requires registration when no targets are specified.

Looks good, in general. Just one more question: will tgt_register_requires be emitted if only -fopenmp is used? If -fopenmp-targets is not provided, this function should not be called, I think, because we're not going to use offloading at all in this case and tgt_register_requires should not be emitted. Otherwise, we may end up with the linker error that tgt_register_requires is not found. Would be good to have a test with -fopenmp and #pragma omp requires that check that no tgt_register_requires is emitted in this case.

Done with test included.

ABataev added inline comments.May 21 2019, 6:28 AM
lib/Frontend/CompilerInvocation.cpp
2840 ↗(On Diff #200355)

We already have Opts.OMPTargetTripples. We use it currently to check if the offloading enabled !CGM.getLangOpts().OMPTargetTriples.empty(), so, probably, we don't need this new flag.

gtbercea marked an inline comment as done.May 21 2019, 6:57 AM
gtbercea added inline comments.
lib/Frontend/CompilerInvocation.cpp
2840 ↗(On Diff #200355)

Even better!! I'll make the change!

gtbercea updated this revision to Diff 200504.May 21 2019, 7:26 AM
  • Remove new option.
ABataev accepted this revision.May 21 2019, 7:37 AM

LG. Please, commit it only after the runtime part is committed.

gtbercea marked an inline comment as done.May 21 2019, 7:39 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2019, 12:39 PM