This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Use AAHeapToStack/AAHeapToShared analysis in SPMDization
ClosedPublic

Authored by ggeorgakoudis on Jul 8 2021, 7:34 AM.

Details

Summary

SPMDization D102307 detects incompatible OpenMP runtime calls to abort converting a target region to SPMD mode. Calls to memory allocation/de-allocation routines kmpc_alloc_shared, kmpc_free_shared are incompatible unless they are removed by AAHeapToStack/AAHeapToShared analysis. This patch extends SPMDization detection to include AAHeapToStack/AAHeapToShared analysis results for enlarging the scope of possible SPMDized regions detected.

Diff Detail

Event Timeline

ggeorgakoudis created this revision.Jul 8 2021, 7:34 AM
ggeorgakoudis requested review of this revision.Jul 8 2021, 7:34 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
ggeorgakoudis edited the summary of this revision. (Show Details)Jul 8 2021, 7:52 AM
jhuber6 added inline comments.Jul 8 2021, 7:57 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
2567

There should only ever be one free call per variable, it's like a stack. This is here just to make sure we don't accidentally remove something that's behind a PHI or something.

2614–2621

We could remove this if we track the free calls separately, but we'd need to have some mapping from the allocation to its corresponding free.

Test ;)

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
2567

Initially we have one free per alloc and the free uses the alloc, transformations can change that, e.g. introduce phi nodes or sink free into conditionals. We need to be conservative if the structure is not as we expect it.

3456–3459

One is sufficient, if the other becomes invalid we don't have to give up. In general, we should not give up on all of kernel info if SPMD-zation can't happen. Also, this is not even needed here ;)

3499–3501
3529

indicateOptimisticFixpoint is not correct above.
You use "assumed" information, thus it can change over time. If the information is known you could indicate a fixpoint. Instead, just don't add this call site to the SPMDCompatibilityTracker if we assume it is removed (by one of the h2s AAs).

ggeorgakoudis marked 3 inline comments as done.Jul 8 2021, 8:56 AM
ggeorgakoudis added inline comments.
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
2567

Got it

2614–2621

Agreed

ggeorgakoudis marked 2 inline comments as done.

Fix for comments

ggeorgakoudis marked 3 inline comments as done.Jul 8 2021, 10:22 AM
jhuber6 added inline comments.Jul 8 2021, 11:25 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3498

For HeapToShared I used lookupAAFor because it returns null if the instance doesn't exist or is invalid. Not sure if that is an issue here.

Test?

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3498

heap2shared doesn't depend on heap2stack, it could but it doesn't. Here we want getorcreate because there is a real dependence

jdoerfert accepted this revision.Jul 21 2021, 8:22 PM

Some minor things to address but LGTM.

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

Nit: early exit please.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
2554

Add brief doxygen description please.

2567

Remove the todo, keep the check though.

2589–2593
2600

same as above.

3494

Nit: Add a message.

llvm/test/Transforms/OpenMP/spmdization.ll
68

Rebase and this is probably going to be a 2 not a 0 anymore.

This revision is now accepted and ready to land.Jul 21 2021, 8:22 PM
ggeorgakoudis marked 8 inline comments as done.

Fix for comments

ggeorgakoudis marked an inline comment as done.Jul 22 2021, 3:23 PM
This revision was landed with ongoing or failed builds.Jul 22 2021, 6:08 PM
This revision was automatically updated to reflect the committed changes.

Looks like you've got some missing override markers:

llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2588:8: error: 'isAssumedHeapToShared' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
  bool isAssumedHeapToShared(CallBase &CB) const {
       ^
llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2520:16: note: overridden virtual function is here
  virtual bool isAssumedHeapToShared(CallBase &CB) const = 0;
               ^
llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2592:8: error: 'isAssumedHeapToSharedRemovedFree' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
  bool isAssumedHeapToSharedRemovedFree(CallBase &CB) const {
       ^
llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2524:16: note: overridden virtual function is here
  virtual bool isAssumedHeapToSharedRemovedFree(CallBase &CB) const = 0;
               ^
2 errors generated.

Looks like you've got some missing override markers:

llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2588:8: error: 'isAssumedHeapToShared' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
  bool isAssumedHeapToShared(CallBase &CB) const {
       ^
llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2520:16: note: overridden virtual function is here
  virtual bool isAssumedHeapToShared(CallBase &CB) const = 0;
               ^
llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2592:8: error: 'isAssumedHeapToSharedRemovedFree' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
  bool isAssumedHeapToSharedRemovedFree(CallBase &CB) const {
       ^
llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2524:16: note: overridden virtual function is here
  virtual bool isAssumedHeapToSharedRemovedFree(CallBase &CB) const = 0;
               ^
2 errors generated.

I'll fix it real quick

I'll fix it real quick

Thanks!