This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Base support for target directive codegen on NVPTX device.
ClosedPublic

Authored by arpith-jacob on Mar 3 2016, 7:35 PM.

Diff Detail

Event Timeline

arpith-jacob retitled this revision from to [OpenMP] Base support for target directive codegen on NVPTX device..
arpith-jacob updated this object.
arpith-jacob added reviewers: ABataev, hfinkel.
ABataev added inline comments.Mar 3 2016, 9:10 PM
lib/CodeGen/CGOpenMPRuntime.cpp
3790

Revert back

3796–3808

What is the purpose of these massive changes?

3918–3919

Restore it back, please.

lib/CodeGen/CGOpenMPRuntime.h
50–54

No way!!! Revert it back, please. No need to expose all these stuff in header file

383

Please, gather all 'protected' members in a single 'protected' section

lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
30

If you expect 'llvm::Function*' here, why you can't change it to 'llvm::Function*'?

47

Remove, not required

133

Comment for 'true' value

242

What about exceptions? Do you plan to support them? If yes, add tests for classes with constructors/destructors and exceptions

lib/CodeGen/CGOpenMPRuntimeNVPTX.h
30

Please, gather all private members in single 'private' section, protected to single 'protected' and public to single 'public'

36–41

No way!!! This must go to .cpp file!

68–102

Move all implementations to .cpp file

117–126

Implementation must be in .cpp

129–131

Again, move it to .cpp

138–172

You can leave just declaration here, as this class is used by reference always. Definition must be moved to .cpp

Addressed feedback; see inline comments for details.

arpith-jacob marked 15 inline comments as done.Mar 4 2016, 2:02 PM
arpith-jacob added inline comments.
lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
27

This function will also handle global variables from the 'declare target' construct in the future.

void CGOpenMPRuntimeNVPTX::createOffloadEntry(llvm::Constant *ID,

                                            llvm::Constant *Addr,
                                            uint64_t Size) {
auto *F = dyn_cast<llvm::Function>(Addr);
// TODO: Add support for global variables on the device after declare target
// support.
if (!F)
  return;
27

What about exceptions? Do you plan to support them? If yes, add tests for classes with constructors/destructors and exceptions

This backend does not support exceptions. We plan to send a separate patch that will selectively deactivate exceptions in target regions generated for the GPU.

ABataev added inline comments.Mar 8 2016, 10:32 PM
lib/CodeGen/CGOpenMPRuntime.h
67

Missed 'virtual'

lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
26

Use 'llvm::None' instead of '{}'

35

Use 'llvm::None' instead of '{}'

44

Use 'llvm::None' instead of '{}'

52

Remove '{}', not required

89

Just 'namespace', remove 'anonymous'

103

Use '.arrangeNullaryFunction()' instead

234

Use 'llvm::None' as the second arg

lib/CodeGen/CGOpenMPRuntimeNVPTX.h
26

All functions must start with a lower case letter

arpith-jacob marked 2 inline comments as done.

Stylistic changes to address feedback.

arpith-jacob marked 9 inline comments as done.Mar 9 2016, 7:59 AM
arpith-jacob added inline comments.
lib/CodeGen/CGOpenMPRuntime.h
67

Do we need 'virtual'? This function needs to be called by the NVPTX implementation (in emitTargetOutlinedFunction) but doesn't need to be overridden.

ABataev added inline comments.Mar 9 2016, 7:27 PM
lib/CodeGen/CGOpenMPRuntime.h
67

I we don't need it in 'CGOpenMPRuntime', then why it is here?

arpith-jacob marked an inline comment as done.Mar 9 2016, 7:44 PM
arpith-jacob added inline comments.
lib/CodeGen/CGOpenMPRuntime.h
67

Alexey, both CGOpenMPRuntime and CGOpenMPRuntimeNVPTX call emitTargetOutlinedFunctionHelper().

CGOpenMPRuntime::emitTargetOutlinedFunction() sets up the 'codegen' lambda and then calls CGOpenMPRuntime::emitTargetOutlinedFunctionHelper() to do the work.

In the same way, CGOpenMPRuntimeNVPTX::emitTargetOutlinedFunction() sets up the 'codegen' lambda for the GPU and then calls CGOpenMPRuntime::emitTargetOutlinedFunctionHelper().

Does that make sense?

ABataev accepted this revision.Mar 9 2016, 8:20 PM
ABataev edited edge metadata.

LG

This revision is now accepted and ready to land.Mar 9 2016, 8:20 PM
This revision was automatically updated to reflect the committed changes.
mkuron added a subscriber: mkuron.Mar 20 2016, 9:12 AM