added custom parser and pretty printer for target construct with if, thread_limit, nowait, and device clauses
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Requesting a few changes.
| mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
|---|---|---|
| 100 | Nit: Is there an extra space before '=' now? | |
| 260–301 | You can remove this and add D102816 as a parent patch. | |
| mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
| 26 | Is this required? | |
| 790–801 | This build function is probably not required. The autogenerated one will probably do this I guess. Do you need this for a particular reason? | |
| 884 | Nit: Please add an empty line after this function. | |
| mlir/test/Dialect/OpenMP/ops.mlir | ||
| 93 | Nit: Accidental change? | |
| 166–167 | Nit: some formatting accidentally added? | |
| 284–297 | This test is using the generic syntax and probably not required. | |
| 304 | Nit: One or two spaces at the beginning here. | |
| 308 | Add the if, thread_limit, nowait tests here or in another omp.target op. | |
| 309 | Nit: One or two spaces at the beginning here. | |
| mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
|---|---|---|
| 790–801 | +1 | |
| 809 | ||
| 810 | nit: added spaces just to fit with previous style | |
| 831 | Might be good to have these keywords declared as constexpr somewhere (extraClassDeclaration) so you can reuse them in the printer. | |
| 857 | Would be nice to have the attribute name declared in the class and not hard-coded here. It's pretty sure that it will be used in conversion and translation. | |
| mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
|---|---|---|
| 100 | No. I will correct it. | |
| 260–301 | I have made D102816 a parent task. However, I am introducing let parser = [{ return parseTargetOp(parser, result); }]; here. | |
| mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
| 26 | No, I will remove it. | |
| 810 | OK. | |
| 831 | OK. Should we do it to the other operations as well? ParallelOps | |
| 857 | OK | |
| 884 | OK | |
| mlir/test/Dialect/OpenMP/ops.mlir | ||
| 93 | Yes.. Will fix it | |
| 166–167 | Got it. | |
| mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
|---|---|---|
| 808 | I will change it to SmallVector<int,3> | |
| mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
|---|---|---|
| 26 | What's the reason to remove this blank line? | |
| 171 | Unrelated change? | |
| 808 | It's recommended to not specify the number of inlined elements for SmallVector unless you have a good reason. I would just use SmallVector<unsigned> in this case. | |
| 813 | Could be constexpr? | |
Add spaces which were accidently removed OpenMPDialect.cpp
Change SmallVector<int,3> to SmallVector<int>
Change const int to constexpr int
Made changes as per reviewer's comments
Looks mostly OK. A few more comments.
| mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
|---|---|---|
| 279 | Nit: Spelling : eliminates | |
| 293 | I think this builder is doing the same thing that the default builder does. So is it required? Also, the if_expr, device and thread_limit are operands and not attributes. So I think their type should be Value and not IntegerAttr. | |
| mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
| 795 | Nit: ssa-id -> ssa-id-and-type ? | |
| 865–866 | I guess @clementval's SmallVector comment about not specifying the size applies here as well. | |
There are still some comments not addressed so far. I guess once you have addressed all the comments we can move forward,
| mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
|---|---|---|
| 371 | Seems like unrelated change. | |
| mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
|---|---|---|
| 293 | Yeah I think you can remove it. Seems like you don't need a specific builder for the time being. | |
Hi @abidmalikwaterloo Thanks for this patch. There are a couple of issues with the styling which is causing the builds to fail. For example trailing spaces, 80-character line wraps and maybe some indentation. I have a few comments.
In the OpenMP 5.0 standard, there is no thread_limit clause for the target construct. Is it necessary?
| mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
|---|---|---|
| 295 | These functions are used only in the parser, and they can be removed - the strings can directly be used. (You will not have to use them too once you use the parseClauses function). | |
| mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
| 817 | This code can now be condensed using the parseClauses function in the latest version of the file (like SectionsOp and ParallelOp). | |
@shraiysh I think this patch is following OpenMP 5.1 standard. In line 261 of OpenMPOps.td, section 2.14.5 is refered and it is the section in openmp 5.1 standard.
| mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
|---|---|---|
| 264 | Should there be one space between "target", and [AttrSizedOperandSegments]? | |
| 287 | The device modifier is missed. Will it be implemented later? | |
Unfortunately, there have been several developments since we last looked at this patch. This includes using assembly format for the printer/parser instead of the custom printTargetOp/parseTargetOp. See the following for reference.
https://github.com/llvm/llvm-project/blob/6f73bd781305266a747055875ce8352e5a36c809/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td#L115
| mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
|---|---|---|
| 286 | You can use I1 here instead of AnyInteger. | |
| mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
|---|---|---|
| 817 | Should we need to do it? I can come to it later for another | |
| mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
|---|---|---|
| 817 | Not anymore. We should use the assemblyformat now and eliminate custom parsers entirely. | |
| mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
|---|---|---|
| 817 | I see the assembly format for the target operation. Does this mean the patch is redundant? | |
Nit: Is there an extra space before '=' now?