Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
68–70 | Nit: "variable" is unclear in MLIR context, we only have values. Also please terminate sentences with a dot, this is required for documentation, comments, etc. except error messages. | |
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
73 | Nit: please use backticks consistently (here and above) | |
137 | Nit: prefer early-returns, i.e. if (varAllocate.empty()) return; <current-code-with-less-indentation> | |
138–140 | Nit: this does not look properly formatted | |
141–145 | mlir::interleaveComma will make it easier to read | |
200–205 | SmallVector is re-exported into the mlir namespace, please drop llvm:: here and above. | |
342 | Nit: drop trivial braces |
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
---|---|---|
88 | Is it possible for us to "fix" the type of allocate, since we know it'll always be an intptr_t? Genuine question, I'm not sure if this is possible/desirable in mlir. | |
138–140 | I also checked this because it looked odd, but this is how clang-format leaves this for me as well. | |
200–205 | Shall we remove the llvm:: prefix on the existing variables as a separate patch, so as not to expand the scope of this one? I can submit that patch since those SmallVectors were added by me in the first place. | |
mlir/test/Dialect/OpenMP/ops.mlir | ||
102 | Question in general for these clauses, is it possible to elide any of these types or do they need to be specified each time? This applies to the other parallel clauses as well as just this one: when I was writing those I couldn't find a way to elide the types. Generally I think it doesn't make those ones that much more verbose, but here it does make them quite verbose due to the necessary "->" syntax etc. We might find this verbosity gets even worse for future clauses on other omp directives. |
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
---|---|---|
88 | MLIR's equivalent of intptr_t is index, but I suppose this also needs to be compatible with LLVM dialect integers. | |
138–140 | Would it re-split lines if this had been p << " " << "allocate" << "("; ? It also looks like p << " allocate("; is just fine here. | |
200–205 | Yes please! | |
mlir/test/Dialect/OpenMP/ops.mlir | ||
102 | It's possible to omit types in the pretty syntax if the parser can infer them within the scope of one operation. E.g., load/store expect subscript types to be index so they can just call parser.getBuilder().getIndexType() instead of requiring it to be present in the assembly form. |
mlir/test/Dialect/OpenMP/ops.mlir | ||
---|---|---|
102 | So, it is possible to omit them in e.g. the if clause that I added before (since we know that the condition is always an i1) but not possible for example in copyin where the type is AnyType? Even though that type is set when that variable is passed into the function itself? |
mlir/test/Dialect/OpenMP/ops.mlir | ||
---|---|---|
102 | Ultimately you need to be able to construct an operation given only the information present in the assembly syntax. At parsing time, the operand values may not have been created yet, so you cannot query their type. |
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
---|---|---|
138–140 | I'll try this and see if it formats it weird again. |
There are review comments that remain unaddressed on this. Please address or comment as to why you chose not to address them.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
---|---|---|
141–145 | I've tried to use that and it seemed to make the code more complex and so l decided not to use it. | |
342 | When adding the allocate and allocator parameters I followed the structure of the already implemented clauses, which use braces, so I thought it would be a good idea to keep those the same. |
Comments inline.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
---|---|---|
135 | There is a mismatch in the order of arguments between the caller and the callee here. | |
342 | I believe this suggestion is as per llvm coding standards. Agree that there is other code which also needs to be changed. Perhaps in a different patch. |
LGTM subject to addressing one comment inline. Would it be possible to add an error check for allocate parsing?
Please wait for approval from other reviewers.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
90 | Nit question: Should this be renamed to verifyParallelOp? | |
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
70–73 | Unless I am missing something, this needs correction. Suggestion below, please consult other examples and suitably modify before use. /// operand-and-type-list ::= `(` allocate-operand-list `)` /// allocate-operand-list :: = allocate-operand | allocate-operand `,` allocate-operand-list /// allocate-operand :: = ssa-id-and-type -> ssa-id-and-type /// ssa-id-and-type ::= ssa-id `:` type |
Since it's already approved, feel free to remove test case modification and commit no need for another revision :)
EDIT: if other reviewers comments are already addressed.
flang/unittests/Lower/OpenMPLoweringTest.cpp | ||
---|---|---|
86 | This unit-tests was removed in https://reviews.llvm.org/rGbe1197c403b22291e35cbc5e96788860ceabd40c |
flang/lib/Lower/OpenMP.cpp | ||
---|---|---|
112 ↗ | (On Diff #297263) | Here these are just a placeholder for the allocate and allocator. They will be replaced with the actual lowering implementation for the allocate clause in my next patch. |
flang/lib/Lower/OpenMP.cpp | ||
---|---|---|
112 ↗ | (On Diff #297263) | This may cause conflict in FIR-dev. Could you please take a look ? |
This unit-tests was removed in https://reviews.llvm.org/rGbe1197c403b22291e35cbc5e96788860ceabd40c