[PATCH] D60568: [OpenMP] Add support for registering requires directives with the runtime
Alexey Bataev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 16 12:37:54 PDT 2019
ABataev added inline comments.
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8975
+ "Target region emitted before requires directive.");
+ HasRequiresUnifiedSharedMemory = true;
The message speaks about target region but points to the clause. You need to correct the message.
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9065
+ // contain at least 1 target region.
+ if (HasRequiresUnifiedSharedMemory && hasEmittedTargetRegion)
+ Flags = OMP_REQ_UNIFIED_SHARED_MEMORY;
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?
Comment at: lib/CodeGen/CGOpenMPRuntime.h:644
+ /// Flag for keeping track of weather a target region has been emitted.
+ bool hasEmittedTargetRegion = false;
> gtbercea wrote:
> > ABataev wrote:
> > > gtbercea wrote:
> > > > gtbercea wrote:
> > > > > ABataev wrote:
> > > > > > gtbercea wrote:
> > > > > > > ABataev wrote:
> > > > > > > > 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.
> > > > > > > This does not determine if the function is called or not. This helps determine the flags with which the function is called.
> > > > > > It does not matter, it still does not work correctly if the target region is called in the function from another module
> > > > > If the target is in another compilation unit, that unit will need to have a requires directive.
> > > > If you can come up with an example which you think doesn't work I'm happy to try it.
> > > 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?
> > 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.
> 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.
Must start from capital letter
CHANGES SINCE LAST ACTION
More information about the cfe-commits