This is an archive of the discontinued LLVM Phabricator instance.

[mlir][OpenMP] Added assembly format for omp.wsloop and remove parseClauses
ClosedPublic

Authored by shraiysh on Mar 10 2022, 2:52 AM.

Details

Summary

This patch

  • adds assembly format for omp.wsloop operation
  • removes the parseClauses clauses as it is not required anymore

This is expected to be the final patch in a series of patches for replacing
parsers for clauses with oilist.

Diff Detail

Event Timeline

shraiysh created this revision.Mar 10 2022, 2:52 AM
Herald added a project: Restricted Project. · View Herald Transcript
shraiysh requested review of this revision.Mar 10 2022, 2:52 AM
clementval added inline comments.Mar 10 2022, 4:26 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
327

Why adding for?

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
74

seems like an unrelated change.

shraiysh updated this revision to Diff 414346.Mar 10 2022, 4:45 AM

Removed unrelated reorganization.

shraiysh marked an inline comment as done.Mar 10 2022, 4:45 AM
shraiysh added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
327

for was added to make it more readable. It isn't necessary, but it avoids confusion with too many parentheses and clauses. I thought the first snippet has more readability than the second one -

omp.wsloop nowait inclusive
for (%iv) : index = (%lb) to (%ub) step (%step)
omp.wsloop nowait inclusive
(%iv) : index = (%lb) to (%ub) step (%step)

I don't mind removing it though, if this feels unneeded.

peixin added inline comments.Mar 10 2022, 4:58 AM
mlir/test/Dialect/OpenMP/invalid.mlir
91–93

This is not good message. "for" is there. The right message should be invalid clause "inclusive".

shraiysh added inline comments.Mar 10 2022, 5:16 AM
mlir/test/Dialect/OpenMP/invalid.mlir
91–93

The error is shown as following (with a pointing ^ on inclusive) - where for is expected. Thus it will give user an indication where for is expected. This message is auto-generated by the generated parser for oilist. I don't think there is an easy way to modify the message.

sample.mlir:3:21: error: custom op 'omp.wsloop' expected 'for'
  omp.wsloop nowait inclusive for (%iv) : index = (%lb) to (%ub) step (%step) {
                    ^
peixin added inline comments.Mar 10 2022, 11:05 PM
mlir/test/Dialect/OpenMP/invalid.mlir
91–93

Well. Each design has pros and cons. It is ok for me if it is not easy to modify the message. Neverthless, the semantic checks can guarantee us one correct input for source code.

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

Since the operation has a loop in its name (wsloop) the 'for' seems extraneous. Can you move the loop index, start, end, step to the beginning right after wsloop?

shraiysh added inline comments.Mar 11 2022, 1:22 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
327

That is because we cannot separate region arguments (index) from the region itself in assembly format (at least not yet). So, they have to be parsed in the same custom<WsLoopControl> function. (and with for, it is similar to C/Fortran code 😄). If there are any suggestions to tackle this issue, please let me know, I will change it accordingly.

mlir/test/Dialect/OpenMP/invalid.mlir
91–93

Yes😄

shraiysh updated this revision to Diff 416538.Mar 18 2022, 9:57 AM

Ping. Rebase with main.

bondhugula added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
311

It looks like the example snippet in the description isn't in sync with this syntax?

Mogball accepted this revision.Mar 21 2022, 1:26 PM
Mogball added inline comments.
mlir/lib/Conversion/SCFToOpenMP/SCFToOpenMP.cpp
416 ↗(On Diff #416538)
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
553

drop trivial braces

This revision is now accepted and ready to land.Mar 21 2022, 1:26 PM
shraiysh updated this revision to Diff 417192.Mar 21 2022, 10:51 PM
shraiysh marked 6 inline comments as done.

Addressed comments. Rebase with main.

Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 10:51 PM