Page MenuHomePhabricator

[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
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
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.

7956 ↗(On Diff #195608)

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

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