This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Reorganize code to allow specialized code generation for different devices.
ClosedPublic

Authored by sfantao on Feb 1 2016, 12:39 PM.

Details

Summary

Different devices may in some cases require different code generation schemes in order to implement OpenMP. This is required not only for performance reasons, but also because it may not be possible to have the current (default) implementation working for these devices. E.g. GPU's cannot implement the same scheme a target such as powerpc or x86b would use, in the sense that it does not have the ability to fork threads, instead all the threads are always executing and need to be managed by the implementation.

This patch proposes a reorganization of the code in the OpenMP code generation to pave the way to have specialized implementation of OpenMP support. More than a "real" patch this is more a request for comments in order to understand if what is proposed is acceptable or if there are better/easier ways to do it.

In this patch part of the common OpenMP codegen infrastructure is moved to a new file under a new namespace (CGOpenMPCommon) so it can be shared between the default implementation and the specialized one. When CGOpenMPRuntime is created, an attempt to select a specialized implementation is done.

In the patch a specialization for nvptx targets is done which currently checks if the target is an OpenMP device and trap if it is not.

Let me know comments suggestions you may have.

Diff Detail

Event Timeline

sfantao updated this revision to Diff 46566.Feb 1 2016, 12:39 PM
sfantao retitled this revision from to [OpenMP] Reorganize code to allow specialized code generation for different devices..
sfantao updated this object.
ABataev added inline comments.Feb 2 2016, 11:20 PM
lib/CodeGen/CGOpenMPRuntimeCommon.h
1 ↗(On Diff #46566)

I don't think we need this new file and new namespace. If some (currently) internal classes are needed, they must be exposed via CGOpenMPRuntime class. Derived class will be able to use these classes.
Also, do not expose classes if they are not required right now.

267–268 ↗(On Diff #46566)

I think it is better to make this function a member of CodeGenFunction.

270–279 ↗(On Diff #46566)

If you need this one, make it a virtual member of CGOpenMPRuntime

lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
25

I think it must be checked during creation of NVPTX specific OpenMPRuntime instance.

sfantao updated this revision to Diff 46825.Feb 3 2016, 1:18 PM
sfantao marked 4 inline comments as done.

Clean up changes that are not required now.

Use CGOpenMPRuntime to contain everything that requires sharing.

Create diagnostic to notify user about unsupported OpenMP targets.

Hi Alexey,

Thanks again for the review!

lib/CodeGen/CGOpenMPRuntimeCommon.h
1 ↗(On Diff #46566)

Alright. I will add what is required as protected members of CGOpenMPRuntime. For now I didn't do that for any of the classes given that we are not doing any actual codegen.

267–268 ↗(On Diff #46566)

Ok, once we start moving things to CGOpenMPRntime runtime, we add this to CodeGenFunction accordingly.

270–279 ↗(On Diff #46566)

Ok. I'll do that once we post the codegen patches.

lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
25

Ok, I did that. Also I added a new diagnostic in the compiler invocation so that the user gets a message instead of a stack dump. Let me know if you agree.

ABataev added inline comments.Feb 3 2016, 10:19 PM
lib/CodeGen/CGOpenMPRuntime.cpp
340–341

I think it is better to reorganize this code like that:

switch (CGM.getTarget().getTriple().getArch()) {
  case llvm::Triple::nvptx:
  case llvm::Triple::nvptx64:
    assert(CGM.getLangOpts().OpenMPIsDevice &&
           "OpenMP NVPTX is only prepared to deal with device code.");
    return new CGOpenMPRuntimeNVPTX(CGM);
  default:
    break;
}
return new CGOpenMPRuntime(CGM);
lib/CodeGen/CGOpenMPRuntime.h
552–555

Do you really need this function? Currently, I don't see a point in adding this platform-specific thing to (mostly) common interface

Hi Alexey,

Thanks for the feedback!

lib/CodeGen/CGOpenMPRuntime.cpp
340–341

Ok, will do.

lib/CodeGen/CGOpenMPRuntime.h
552–555

I am using that function to avoid exposing the constructor of CGOpenMPRuntimeNVPTX in CGOpenMPRuntime.h. Do you prefer me to do that instead?

ABataev added inline comments.Feb 3 2016, 10:38 PM
lib/CodeGen/CGOpenMPRuntime.h
552–555

Why do you need to expose constructor of CGOpenMPRuntimeNVPTX in CGOpenMPRuntime.h? It must be exposed only in CGOpenMPRuntime.cpp. Also, it would be a good idea to remove CGOpenMPRuntime::create() and move the whole logic to CodeGenModule::createOpenMPRuntime()

sfantao added inline comments.Feb 3 2016, 10:52 PM
lib/CodeGen/CGOpenMPRuntime.h
552–555

Ok I can move things to CodeGenModule.

I am afraid I am not understanding what you want me to do with CGOpenMPRuntimeNVPTX constructor. We have CGOpenMPRuntimeNVPTX in CGOpenMPRuntimeNVPTX.cpp, so unless I declare it in CGOpenMPRuntime.h (or some other header file) or have a function declared somewhere and defined in CGOpenMPRuntimeNVPTX.cpp, I can't call the Ctor.

So do you want me do create two functions in the CodeGen namespace (say createDefaultOpenMPRuntime() and createNVPTXOpenMPRuntime) and use them in CodeGenModule after checking which target is being used?

ABataev added inline comments.Feb 3 2016, 11:09 PM
lib/CodeGen/CGOpenMPRuntime.h
552–555

Oh, I missed that you did not add header file. I think you need to add a new one for new runtime support library.

sfantao updated this revision to Diff 46935.Feb 4 2016, 11:35 AM

Create CGOpenMPRuntimeNVPTX header file and mike specialization selection in CodeGenModule.

ABataev added inline comments.Feb 4 2016, 11:40 AM
lib/CodeGen/CodeGenModule.cpp
217

I think this must be removed

lib/Frontend/CompilerInvocation.cpp
1837

OpenMPIsDevice is always false here. Do we really need it here?

sfantao updated this revision to Diff 46937.Feb 4 2016, 11:47 AM
sfantao marked 2 inline comments as done.

Remove return statement and use 'false' instead of relying on OpenMPIsDevice.

lib/CodeGen/CodeGenModule.cpp
217

Ok, done.

lib/Frontend/CompilerInvocation.cpp
1837

Ok, hardcoded it to false.

sfantao updated this revision to Diff 46939.Feb 4 2016, 11:51 AM

Add comment.

ABataev added inline comments.Feb 4 2016, 11:59 AM
lib/Frontend/CompilerInvocation.cpp
1837

No, I mean do we really need this argument if this is a constant? I think for now we should just throw it away and edit error message so that it does not expect the second argument,

sfantao updated this revision to Diff 46941.Feb 4 2016, 12:04 PM

Change diagnostic for invalid host target.

sfantao marked an inline comment as done.Feb 4 2016, 12:05 PM
sfantao added inline comments.
lib/Frontend/CompilerInvocation.cpp
1837

Ok, I did as you say. thanks!

ABataev accepted this revision.Feb 4 2016, 7:43 PM
ABataev edited edge metadata.

LG

This revision is now accepted and ready to land.Feb 4 2016, 7:43 PM
sfantao closed this revision.Feb 5 2016, 6:16 PM
sfantao marked an inline comment as done.