This is an archive of the discontinued LLVM Phabricator instance.

Header file to help forcibly link GPURuntime
ClosedPublic

Authored by singam-sanjay on May 15 2017, 7:37 AM.

Details

Summary

LinkGPURuntime.h defines and creates a structure ForceGPURuntimeLinking which creates an artificial dependency to functions defined in GPUJIT.c. The presence of this structure ensures that these functions are a part of the compiled object/library files including it.

Diff Detail

Repository
rL LLVM

Event Timeline

singam-sanjay created this revision.May 15 2017, 7:37 AM
grosser requested changes to this revision.May 15 2017, 7:53 AM

Otherwise, this looks good.

include/polly/Support/LinkGPURuntime.h
1 ↗(On Diff #99004)

The FOLDER tag looks surprising.

This revision now requires changes to proceed.May 15 2017, 7:53 AM

I've uploaded only a proof-of-concept version. @grosser @Meinersbur Need your suggestions with code style, location of the file, etc. Questions inlined.

CMakeLists.txt
209 ↗(On Diff #99004)

This line makes available the files under tools/GPURuntime.

This is for a case when a program within the LLVM tree (opt, bugpoint) would require ForceGPURuntimeLinking.h. It isn't useful when the file is to be accessed by an external entitiy, e.g. Julia, since the line(s) that'll include <polly_src>/tools will have to be added to the compile commands of the tool.

include/polly/Support/LinkGPURuntime.h
1 ↗(On Diff #99004)

Which folder would you recommend placing this file ? It's currently in include/polly/Support.

18 ↗(On Diff #99004)

Although the Julia (the external tool) compiled without this construct, would it be better to enclose the #include line inside this extern "C" since it specifies that the code within should be treated as C code ?

e.g. Lines like the following, in GPUJIT.c, which are allowed in the default C dialect used by GCC, would fail when compiled as C++ ( although it didn't this time. Why was that the case ? )

34 ↗(On Diff #99004)

Should I typecast the nullptr (like in the line below) or leave it as is ?

singam-sanjay edited edge metadata.
  1. Stylistic changes
  2. Used static structure instead of func call to force link.
singam-sanjay marked an inline comment as done.May 16 2017, 6:40 AM
singam-sanjay added inline comments.
include/polly/Support/LinkGPURuntime.h
34 ↗(On Diff #99004)

I've removed all pointer and integer type casts.

43 ↗(On Diff #99134)

I'm now using a static structure variable instead a function call to force-link, like LinkAllIR.h. This change would make it simpler to force-link by only requiring a #include line for LinkGPURuntime.h .

@grosser Could you please review this patch ? Julia has accepted my PR to introduce the USE_POLLY_ACC option and uses LinkGPURuntime.h to Link GPURuntime into libjulia.so. Please leave your comments to get this patch working ASAP, before someone tries out USE_POLLY_ACC:=1.

Hi Sanjay,

this looks overall good. However, there is one question open:

include/polly/Support/LinkGPURuntime.h
18 ↗(On Diff #99004)

Why are you including the full .c file? You should probably introduce a GPUJIT.h header file and only include this one. Like this there is also no need to make CUDA or OpenCL includes available when linking.

singam-sanjay added inline comments.May 23 2017, 12:40 AM
include/polly/Support/LinkGPURuntime.h
18 ↗(On Diff #99004)

I did it this way having Julia in mind. If I had to #include just the GPUJIT.h, I'd have to link libGPURuntime.so to libjulia.so, which would require the rpath to libGPURuntime.so's location. But, the reviewers weren't happy with explicitly adding the rpath. So, I included GPUJIT.c and called every function to ensure an equivalent of linking to GPURuntime happend when I included LinkGPURuntime.h.

I'm now working to see if I could do away with GPUJIT.c and make libGPURuntime.so available by copying it to locations specified in RPATH of libjulia.so.

singam-sanjay updated this revision to Diff 99895.EditedMay 23 2017, 6:27 AM

Not pulling in entire GPUJIT.c anymore. Has to be made available by linking to libGPURuntime.

grosser accepted this revision.May 23 2017, 6:28 AM

This LGTM. Should I commit it?

This revision is now accepted and ready to land.May 23 2017, 6:28 AM
singam-sanjay edited the summary of this revision. (Show Details)May 23 2017, 6:29 AM
singam-sanjay removed a subscriber: mgorny.

This LGTM. Should I commit it?

If you're asking me, please go ahead.

This revision was automatically updated to reflect the committed changes.