Page MenuHomePhabricator

[MLIR][OpenMP] Distribute Construct Operation
Needs ReviewPublic

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

Details

Summary

OpenMP distribute construct for MLIR operation and added a test case.

Diff Detail

Event Timeline

abidmalikwaterloo requested review of this revision.Jul 7 2021, 12:35 PM
abidmalikwaterloo retitled this revision from Added definition for distribute construct for MLIR operation and added a test case. to [MLIR][OpenMP] Distribute Construct Operation.Jul 7 2021, 12:37 PM
abidmalikwaterloo edited the summary of this revision. (Show Details)
kiranchandramohan requested changes to this revision.Aug 10 2021, 10:47 AM

Thanks @abidmalikwaterloo for this patch. This patch needs a cleanup and should not have any unrelated changes. I have added some comments.

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

Nit: Alignment

265

You can remove private, firstprivate and lastprivate for now.

269

Nit: Alignment

287

Unrelated to this patch.

299–300

Unrelated to this patch.

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

These are all unrelated to this patch. Please remove.

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

Removal of this is unrelated to this patch.

This revision now requires changes to proceed.Aug 10 2021, 10:47 AM
clementval requested changes to this revision.Aug 10 2021, 11:20 AM
clementval added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
239

Line should be wrapped at 80.

240

Same here.

250

80

mlir/test/Dialect/OpenMP/ops.mlir
286

Unrelated to this patch

clementval added inline comments.Aug 10 2021, 11:29 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
269

Maybe too permissive for a chunk_size?

This patch has the same problem as the patch https://reviews.llvm.org/D105581/new/. The branching needs to be done from the main. The parsing part should not be there.

rebase the patch branch with the main branch

clementval requested changes to this revision.Wed, Sep 15, 12:51 AM
clementval added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
269

Alignment is still off.

This revision now requires changes to proceed.Wed, Sep 15, 12:51 AM
abidmalikwaterloo marked 8 inline comments as done.Fri, Sep 17, 7:32 AM

Made changes based on the reviewers' comments.

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

Any special reason for this?

269

We can make it AnyInteger. or IntORIndex