Skip to content

Commit

Permalink
[MLIR][OpenMP] More robust support for target SPMD (#161)
Browse files Browse the repository at this point in the history
This patch fixes `TargetOp::getInnermostCapturedOmpOp()` to avoid detecting as
captured by the top target construct other constructs nested inside of a loop.
This prevents the `omp.target` verifier from incorrectly flagging valid SPMD
loops, like in the following example:

```f90
subroutine foo(n)
  implicit none
  integer, intent(in) :: n
  integer :: i, j

  !$omp target teams distribute parallel do
  do i=1,n
    !$omp simd
    do j=1,n
      call bar()
    enddo
  enddo
end subroutine foo
```
  • Loading branch information
skatrak authored Sep 24, 2024
1 parent 968dac4 commit 6d8995a
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 14 deletions.
9 changes: 6 additions & 3 deletions mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
Original file line number Diff line number Diff line change
Expand Up @@ -1127,13 +1127,16 @@ def TargetOp : OpenMP_Op<"target", traits = [
let hasVerifier = 1;

let extraClassDeclaration = [{
/// Returns the innermost OpenMP dialect operation nested inside of this
/// operation's region. For an operation to be detected as captured, it must
/// be inside a (possibly multi-level) nest of OpenMP dialect operation's
/// Returns the innermost OpenMP dialect operation captured by this target
/// construct. For an operation to be detected as captured, it must be
/// inside a (possibly multi-level) nest of OpenMP dialect operation's
/// regions where none of these levels contain other operations considered
/// not-allowed for these purposes (i.e. only terminator operations are
/// allowed from the OpenMP dialect, and other dialect's operations are
/// allowed as long as they don't have a memory write effect).
///
/// If there are omp.loop_nest operations in the sequence of nested
/// operations, the top level one will be the one captured.
Operation *getInnermostCapturedOmpOp();

/// Tells whether this target region represents a single worksharing loop
Expand Down
19 changes: 8 additions & 11 deletions mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1520,6 +1520,13 @@ Operation *TargetOp::getInnermostCapturedOmpOp() {
if (op == *this)
return;

// Reset captured op if crossing through an omp.loop_nest, so that the top
// level one will be the one captured.
if (llvm::isa<LoopNestOp>(op)) {
capturedOp = nullptr;
capturedParentRegion = nullptr;
}

bool isOmpDialect = op->getDialect() == ompDialect;
bool hasRegions = op->getNumRegions() > 0;

Expand Down Expand Up @@ -1563,21 +1570,11 @@ Operation *TargetOp::getInnermostCapturedOmpOp() {

bool TargetOp::isTargetSPMDLoop() {
Operation *capturedOp = getInnermostCapturedOmpOp();

// Allow an omp.atomic_update to be captured inside of the loop and still
// consider the parent omp.target operation to be potentially defining an SPMD
// loop.
// TODO: Potentially accept other captured OpenMP dialect operations as well,
// if they are allowed inside of an SPMD loop.
if (isa_and_present<AtomicUpdateOp>(capturedOp))
capturedOp = capturedOp->getParentOp();

if (!isa_and_present<LoopNestOp>(capturedOp))
return false;

Operation *workshareOp = capturedOp->getParentOp();

// Accept optional SIMD leaf construct.
Operation *workshareOp = capturedOp->getParentOp();
if (isa_and_present<SimdOp>(workshareOp))
workshareOp = workshareOp->getParentOp();

Expand Down

0 comments on commit 6d8995a

Please sign in to comment.