This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Make sure Heap2Stack works properly on a GPU target
ClosedPublic

Authored by jdoerfert on Mar 14 2021, 10:40 AM.

Details

Summary

If the target stack is not accessible between different running
"threads" we have to make sure not to create allocas for mallocs
that might be used by multiple "threads". The "use check" is
sufficient to prevent this but if we apply the "free check" we have
to make sure the pointer is not communicated to others before
the free is reached.

Diff Detail

Event Timeline

jdoerfert created this revision.Mar 14 2021, 10:40 AM
jdoerfert requested review of this revision.Mar 14 2021, 10:40 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: bbn. · View Herald Transcript

Add test case, ensure malloc and free are nosync

jdoerfert abandoned this revision.Mar 14 2021, 7:06 PM
jdoerfert reclaimed this revision.Apr 22 2021, 9:41 AM

I think this is actually not bad.

madhur13490 added inline comments.
llvm/include/llvm/Transforms/IPO/Attributor.h
973

I don't think it is limited to just GPUs. Generally, it is applicable to machines where stack is not shared by multiple threads e.g. this is applicable to X86 threading model too if I am not mistaken - stack is indeed private.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
5135

Typo in "sharable"; "needs to be places" ->placed.

llvm/test/Transforms/Attributor/heap_to_stack_gpu.ll
7

May be have a test "amdgpu" triple too?

jdoerfert marked an inline comment as done.Apr 22 2021, 11:25 AM
jdoerfert added inline comments.
llvm/include/llvm/Transforms/IPO/Attributor.h
973

This might not be only for GPUs but I don't know what else to add. X86 stack is not thread private in the sense that other threads could not access it.

llvm/test/Transforms/Attributor/heap_to_stack_gpu.ll
7

I was already insistent to copy the file once, do we really need to copy it again? I can just make this amdgpu for all I care.

sstefan1 added inline comments.Apr 22 2021, 12:40 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
5019–5020
llvm/test/Transforms/Attributor/heap_to_stack.ll
7 ↗(On Diff #330523)

Should we also annotate these in some cases? AFAIK these will never be nosync right now.

sstefan1 accepted this revision.Apr 22 2021, 12:44 PM
This revision is now accepted and ready to land.Apr 22 2021, 12:44 PM
jdoerfert updated this revision to Diff 352757.Jun 17 2021, 9:21 AM
jdoerfert marked 4 inline comments as done.

address reviews

This revision was landed with ongoing or failed builds.Jun 17 2021, 11:11 PM
This revision was automatically updated to reflect the committed changes.