This is an archive of the discontinued LLVM Phabricator instance.

[Polly] [PPCGCodeGeneration] Add managed memory support to GPU code generation.
ClosedPublic

Authored by bollu on Apr 19 2017, 8:04 AM.

Details

Summary

This needs changes to GPURuntime to expose synchronization between host
and device.

  1. Needs better function naming, I want a better name than

"getOrCreateManagedDeviceArray"

  1. DeviceAllocations is used by both the managed memory and the

non-managed memory path. This exploits the fact that the two code paths
are never run together. I'm not sure if this is the best design decision

Event Timeline

bollu created this revision.Apr 19 2017, 8:04 AM

Hi Siddharth

I really like this idea! Seems like this could be helpful indeed.

However, I do understand it correctly that this should work effortlessly with for example the PolyBench/C kernels, out of the box?
Compiling a number of different kernels, the assertion

865:  assert(false && "multiple allocations for same scop");

gets triggered and the compilation fails with the following error:
clang -O3 -mllvm -polly -mllvm -polly-target=gpu -mllvm -polly-acc-codegen-managed-memory -ldl -lGPURuntime -I utilities -I linear-algebra/solvers/trisolv utilities/polybench.c linear-algebra/solvers/trisolv/trisolv.c -DPOLYBENCH_TIME

{P7990, lines=25}

bollu updated this revision to Diff 95924.Apr 20 2017, 3:38 AM
  • remove debug-time assert that was leftover from debugging
bollu added a comment.Apr 20 2017, 3:42 AM

Yes, I realised this and pushed it to my git. Looks like I forgot to update phab. Thanks for the catch!
That was an assert error that was left out when I was trying to understand different cases.

Also, note that it will not work (fully) out of the box, you would need to change malloc/free to cudaMallocManaged/cudaFree, unless you're running on CUDA 8, in which case it should just work with regular malloc + kernel patches. But yes, the change is minimal

I did modify polybench to run this: [Click link - raw: https://github.com/bollu/polybench-c ](https://github.com/bollu/polybench-c)

bollu added a comment.Apr 20 2017, 3:43 AM

(there is a makefile which can be run to create benchmarks)

Using the modified Polybench/C I can confirm that it's working. When respecting those necessary changes, this patch looks good to me.

grosser edited edge metadata.Apr 20 2017, 4:10 AM

Hey guys,

thanks for pushing this forward. The patch indeed looks already very good. Here, some last some stylistic comments from me.

lib/CodeGen/PPCGCodeGeneration.cpp
511

Only use braces in case there is more than one statement.

654

Missing comment in assert.

674

Start with uppercase.

694

Missing comment.

717

Missing comment.

738

Missing comment.

863

Start with uppercase.

868

use llvm_unreachable

892

Start with uppercase.

954

Don't use braces

965

Here, braces are OK.

bollu updated this revision to Diff 95942.Apr 20 2017, 6:06 AM
  • [NFC with respect to branch] fixed one-line if and assert comments
bollu added inline comments.Apr 24 2017, 12:52 AM
lib/CodeGen/PPCGCodeGeneration.cpp
868

This should be reachable. I hadn't updated the differential with the commit.

965

quick style question because it doesn't seem to be there in the LLVM style guide either.

How do I style

question.cpp
if (predicate) {
    then();
} else {
    else1();
    else2();
}

do I transform this to:

remove-brace-from-then-clause
if (predicate)
    then();
else {
    else1();
    else2();
}

@grosser, @Meinersbur: ping. I've updated comments and fixed if {} styling.

PhilippSchaad accepted this revision.Apr 24 2017, 1:02 AM
This revision is now accepted and ready to land.Apr 24 2017, 1:02 AM
bollu updated this revision to Diff 97080.Apr 28 2017, 4:43 AM

[NoChange] Trying to use LLVM Phab instructions, need to have this commit track arc
patch.

bollu closed this revision.Apr 28 2017, 4:52 AM

Closed by 0cdb52faa664af4427e627b30872d660f177b1d9. I messed up the differential revision number. It should point to D32226 (this revision). Commit message points to [D32225](https://reviews.llvm.org/D32225)