Page MenuHomePhabricator

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

Authored by gtbercea on Thu, Apr 11, 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
ABataev added inline comments.Fri, Apr 12, 7:53 AM
lib/CodeGen/CGOpenMPRuntime.cpp
9056

No need for braces

9060

CGF.CGM.Int64Ty->CGM.Int64Ty

lib/CodeGen/CodeGenModule.cpp
415

You can remove the third nullptr argument

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

Not done

8973

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

9055

Change the type from int64_t to OpenMPOffloadingRequiresDirFlags

lib/CodeGen/CGOpenMPRuntime.h
641

What about this comment?

1445

Can you make it const member function?

gtbercea updated this revision to Diff 195190.Mon, Apr 15, 8:28 AM
  • Fix type.
gtbercea marked an inline comment as done.Mon, Apr 15, 8:28 AM
gtbercea marked 2 inline comments as done.Mon, Apr 15, 8:38 AM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.cpp
8973

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

8973

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

gtbercea updated this revision to Diff 195195.Mon, Apr 15, 8:41 AM
  • Add break.
gtbercea marked an inline comment as done.Mon, Apr 15, 8:41 AM
gtbercea marked 2 inline comments as done.Mon, Apr 15, 8:44 AM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.h
1445

Cannot add that due to createRuntimeFunction not being const.

gtbercea marked 3 inline comments as done.Mon, Apr 15, 8:45 AM
gtbercea updated this revision to Diff 195200.Mon, Apr 15, 8:58 AM
  • Remove const.
ABataev added inline comments.Mon, Apr 15, 9:04 AM
lib/CodeGen/CGOpenMPRuntime.cpp
8973

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.Mon, Apr 15, 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.Tue, Apr 16, 8:48 AM
  • Fix checks for use of requires directive.
gtbercea marked an inline comment as done.Tue, Apr 16, 8:49 AM
ABataev added inline comments.Tue, Apr 16, 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.Tue, Apr 16, 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.Tue, Apr 16, 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.Tue, Apr 16, 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.Tue, Apr 16, 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.Tue, Apr 16, 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.Tue, Apr 16, 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.Tue, Apr 16, 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.Tue, Apr 16, 12:16 PM
gtbercea marked an inline comment as done.Tue, Apr 16, 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.Tue, Apr 16, 12:37 PM
lib/CodeGen/CGOpenMPRuntime.cpp
8975

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

9062

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.Wed, Apr 17, 11:32 AM
gtbercea marked an inline comment as done.
  • Add support for declare target routines.
gtbercea updated this revision to Diff 195609.Wed, Apr 17, 11:41 AM
gtbercea marked an inline comment as done.
  • Update error message.
gtbercea marked 2 inline comments as done.Wed, Apr 17, 11:42 AM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.cpp
8975

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

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

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

lib/CodeGen/CGOpenMPRuntime.cpp
7956

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

8975

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.Wed, Apr 17, 3:29 PM
  • Update tests.
  • Move error check in sema.
ABataev added inline comments.Thu, Apr 18, 6:51 AM
lib/CodeGen/CGOpenMPRuntime.cpp
7956

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.Thu, Apr 18, 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.Thu, Apr 18, 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.Thu, Apr 18, 7:52 AM
  • Add const.
  • Address comments.
gtbercea marked 4 inline comments as done.Thu, Apr 18, 7:53 AM
ABataev added inline comments.Thu, Apr 18, 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.Thu, Apr 18, 10:18 AM
  • Move error check in sema.
gtbercea marked an inline comment as done.Thu, Apr 18, 10:19 AM
ABataev added inline comments.Thu, Apr 18, 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