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
4163–4164

Restore it back, please.

4177

Revert back

4183–4195

What is the purpose of these massive changes?

lib/CodeGen/CGOpenMPRuntime.h
52–56

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

294

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

lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
346

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

363

Remove, not required

449

Comment for 'true' value

558

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

lib/CodeGen/CGOpenMPRuntimeNVPTX.h
137

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

143–148

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

175–209

Move all implementations to .cpp file

224–233

Implementation must be in .cpp

236–238

Again, move it to .cpp

245–279

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
343

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;
343

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
69

Missed 'virtual'

lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
27

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

36

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

45

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

53

Remove '{}', not required

90

Just 'namespace', remove 'anonymous'

104

Use '.arrangeNullaryFunction()' instead

235

Use 'llvm::None' as the second arg

lib/CodeGen/CGOpenMPRuntimeNVPTX.h
26–133

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
69

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
69

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
69

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