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

Repository
rL LLVM

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 ↗(On Diff #49796)

Revert back

3796–3808 ↗(On Diff #49796)

What is the purpose of these massive changes?

3918–3919 ↗(On Diff #49796)

Restore it back, please.

lib/CodeGen/CGOpenMPRuntime.h
50–54 ↗(On Diff #49796)

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

383 ↗(On Diff #49796)

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

lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
30 ↗(On Diff #49796)

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

47 ↗(On Diff #49796)

Remove, not required

133 ↗(On Diff #49796)

Comment for 'true' value

242 ↗(On Diff #49796)

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 ↗(On Diff #49796)

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

36–41 ↗(On Diff #49796)

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

68–102 ↗(On Diff #49796)

Move all implementations to .cpp file

117–126 ↗(On Diff #49796)

Implementation must be in .cpp

129–131 ↗(On Diff #49796)

Again, move it to .cpp

138–172 ↗(On Diff #49796)

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 ↗(On Diff #49851)

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 ↗(On Diff #49851)

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 ↗(On Diff #49851)

Missed 'virtual'

lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
27 ↗(On Diff #49851)

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

36 ↗(On Diff #49851)

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

45 ↗(On Diff #49851)

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

53 ↗(On Diff #49851)

Remove '{}', not required

90 ↗(On Diff #49851)

Just 'namespace', remove 'anonymous'

104 ↗(On Diff #49851)

Use '.arrangeNullaryFunction()' instead

235 ↗(On Diff #49851)

Use 'llvm::None' as the second arg

lib/CodeGen/CGOpenMPRuntimeNVPTX.h
26 ↗(On Diff #49851)

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 ↗(On Diff #50143)

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 ↗(On Diff #50143)

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 ↗(On Diff #50143)

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