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

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
746

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.

746

Add a comment for the whole new type

748

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

2323

Function return type must be void

3871

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

3883

Use getTypes().arrangeNullaryFunction()

3888

Use OpenMPOffloadingRequiresDirFlags instead of int64_t

8254

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

9284

Why do you need the second function?

lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
4882

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
3871

Same as for register lib.

3888

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

8254

I will split it.

9284

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

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

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

1250

No need to do this initialization here

9220

Just HasRequiresUnifiedSharedMemory = true;

9287

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

9303

No need for braces

9307

CGF.CGM.Int64Ty->CGM.Int64Ty

lib/CodeGen/CGOpenMPRuntime.h
1450

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

lib/CodeGen/CodeGenModule.cpp
415

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
9220

Not done

9220

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

9302

Change the type from int64_t to OpenMPOffloadingRequiresDirFlags

lib/CodeGen/CGOpenMPRuntime.h
641

What about this comment?

1450

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
9220

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

9220

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
1450

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
9220

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

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

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

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

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

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

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

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

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

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

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

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
9222

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

9309

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

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
9222

"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
8255

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

9222

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

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
8255

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

Again, this check is not required

test/OpenMP/target_codegen.cpp
99

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
10363

Bad formatting.

10365

No need for the braces

10365

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
10365

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
10365

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
10365

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
10365

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
10365

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
9288

Missed check for HasEmittedTargetRegion here

9309

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");

10365

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