Page MenuHomePhabricator

[MLIR][OpenMP] Teams Construct Operation
AcceptedPublic

Authored by abidmalikwaterloo on Jul 7 2021, 12:25 PM.

Details

Summary

Added the OpenMP Teams Construct operation definition for MLIR.

Diff Detail

Event Timeline

abidmalikwaterloo requested review of this revision.Jul 7 2021, 12:25 PM
abidmalikwaterloo retitled this revision from Added Teams Construct definition for the MLIR operation and added a test case. to [MLIR][OpenMP] Teams Construct Operation.Jul 7 2021, 12:28 PM
abidmalikwaterloo edited the summary of this revision. (Show Details)
clementval added inline comments.Jul 15 2021, 8:20 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
100

From another patch?

142

You might want to align OptionalAttr with the Optional above. I think other operations in this file are like that.

299–300

What's the reason to remove this?

325

From another patch?

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
799 ↗(On Diff #357044)

Should not be removed?

mlir/test/Dialect/OpenMP/ops.mlir
299

Should not be removed?

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
100

I think I need to update my repo.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
100

This is from the previous patch.

299–300

Its accidentally removed because of moving code between branches. I. will fix all these mistakes.

325

yes. It is from another patch.

mlir/test/Dialect/OpenMP/ops.mlir
299

I branched for teams construct. Do we need it for this patch?

Let me submit it again. Most of the stuff is still on my machine but did not appear or seemed removed.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
799 ↗(On Diff #357044)

This is from another patch that is still under review.

abidmalikwaterloo marked an inline comment as done.Jul 15 2021, 6:21 PM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
142

Should we keep it in line with the parallel operation?

mlir/test/Dialect/OpenMP/ops.mlir
299

This is from another patch.

clementval added inline comments.Jul 15 2021, 6:35 PM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
142
142

Just like the suggestion above.

clementval added inline comments.Jul 16 2021, 9:51 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
120

Just a simple question here: I'm not sure we need a 1-to-1 mapping between construct and dialect operation. Did you think through how omp.target and omp.teams operation will interact?

abidmalikwaterloo marked an inline comment as done.Jul 18 2021, 7:20 AM

Here is a simple example of interaction:

func @omp_compute(%x: memref<1024xf32>, %y: memref<1024xf32>,
  %n: index, %a: f32, %N:i32) -> memref<1024xf32> {
  %c0 = constant 0 : index
  %c1 = constant 1 : index
  %c0 = constant 0 : index
  %c1 = constant 1 : index

  omp.target  {
    omp.teams num_teams(2) thread_limit(%N) {
       omp.distribute{
             loop.for %arg0 = %c0 to %n step %c1 {
             %xi = load %x[%arg0] : memref<1024xf32>
             %yi = load %y[%arg0] : memref<1024xf32>
             %ax = mulf %a, %xi : f32
             %yy = addf %ax, %yi : f32
             store %yy, %y[%arg0] : memref<1024xf32>
		}
            }
      }
  }
 return %y : memref<1024xf32>
}

Resumbitting the patch as the previous one was missing some codes

clementval requested changes to this revision.Jul 27 2021, 7:05 PM

I think your patch is still mixing with previous one.

This revision now requires changes to proceed.Jul 27 2021, 7:05 PM

I think your patch is still mixing with the previous one.

@kiranchandramohan
What is the solution to remove this confusionn? This patch does not need the basic parsing patch for target operation. I feel my branching was not correctly done which created this confusion.

I think your patch is still mixing with the previous one.

@kiranchandramohan
What is the solution to remove this confusionn? This patch does not need the basic parsing patch for target operation. I feel my branching was not correctly done which created this confusion.

You likely did your patch from a branch other than main so you have remaining of the previous patches work.
Apply your patch on a fresh branch from main and fix the problematic diffs

I think your patch is still mixing with the previous one.

@kiranchandramohan
What is the solution to remove this confusionn? This patch does not need the basic parsing patch for target operation. I feel my branching was not correctly done which created this confusion.

You likely did your patch from a branch other than main so you have remaining of the previous patches work.
Apply your patch on a fresh branch from main and fix the problematic diffs

OK

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
120

Are you asking with respect to the discussion [[ https://lists.llvm.org/pipermail/llvm-dev/2020-February/139136.html | [llvm-dev] About OpenMP dialect in MLIR]]? Here how I see the interaction:

omp.target {
      omp.teams {
          loop.for{
          }
     }
}

rebase the branch to main to remove the redundent code

abidmalikwaterloo marked 4 inline comments as done.Mon, Sep 13, 6:55 PM
abidmalikwaterloo marked 3 inline comments as done.Thu, Sep 16, 4:51 PM
clementval accepted this revision.Fri, Sep 17, 1:43 AM

Please just update the NIT comment before proceeding.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
123

Please split the line at 80.

This revision is now accepted and ready to land.Fri, Sep 17, 1:43 AM

Made changes to the length of comments as per advised by the reviewer.

abidmalikwaterloo marked an inline comment as done.Fri, Sep 17, 6:52 AM