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
4111–4112

Restore it back, please.

4125

Revert back

4131–4143

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
351

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

368

Remove, not required

454

Comment for 'true' value

563

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
348

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

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

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