This is an archive of the discontinued LLVM Phabricator instance.

[OpenMPIRBuilder] Allocate temporary at the correct block in a nested parallel
ClosedPublic

Authored by wsmoses on Mar 5 2022, 3:30 PM.

Details

Summary

The OpenMPIRBuilder has a bug. Specifically, suppose you have two nested openmp parallel regions (writing with MLIR for ease)

omp.parallel {
  %a = ...
  omp.parallel {
    use(%a)
  }
}

As OpenMP only permits pointer-like inputs, the builder will wrap all of the inputs into a stack allocation, and then pass this
allocation to the inner parallel. For example, we would want to get something like the following:

omp.parallel {
  %a = ...
  %tmp = alloc
  store %tmp[] = %a
  kmpc_fork(outlined, %tmp)
}

However, in practice, this is not what currently occurs in the context of nested parallel regions. Specifically to the OpenMPIRBuilder,
the entirety of the function (at the LLVM level) is currently inlined with blocks marking the corresponding start and end of each
region.

entry:
  ...

parallel1:
  %a = ...
  ...

parallel2:
  use(%a)
  ...

endparallel2:
  ...

endparallel1:
  ...

When the allocation is inserted, it presently inserted into the parent of the entire function (e.g. entry) rather than the parent
allocation scope to the function being outlined. If we were outlining parallel2, the corresponding alloca location would be parallel1.

This causes a variety of bugs, including https://github.com/llvm/llvm-project/issues/54165 as one example.

This PR allows the stack allocation to be created at the correct allocation block, and thus remedies such issues.

Diff Detail

Event Timeline

wsmoses created this revision.Mar 5 2022, 3:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2022, 3:30 PM
wsmoses requested review of this revision.Mar 5 2022, 3:30 PM
wsmoses updated this revision to Diff 413259.Mar 5 2022, 3:31 PM

Add test file

Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2022, 3:31 PM
wsmoses updated this revision to Diff 413309.Mar 6 2022, 12:14 PM

Fix clang test

Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2022, 12:14 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jdoerfert accepted this revision.Mar 6 2022, 2:43 PM

LG, two nits.

llvm/include/llvm/Transforms/Utils/CodeExtractor.h
127–130

Nit: Mention the AllocationBlock here too.

llvm/lib/Transforms/Utils/CodeExtractor.cpp
1195
This revision is now accepted and ready to land.Mar 6 2022, 2:43 PM
This revision was landed with ongoing or failed builds.Mar 6 2022, 3:34 PM
This revision was automatically updated to reflect the committed changes.
wsmoses marked an inline comment as done.