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.
Nit: please use backticks consistently (here and above)
Nit: prefer early-returns, i.e. if (varAllocate.empty()) return; <current-code-with-less-indentation>
Nit: this does not look properly formatted
mlir::interleaveComma will make it easier to read
SmallVector is re-exported into the mlir namespace, please drop llvm:: here and above.
Nit: drop trivial braces
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.
I also checked this because it looked odd, but this is how clang-format leaves this for me as well.
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.
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's equivalent of intptr_t is index, but I suppose this also needs to be compatible with LLVM dialect integers.
Would it re-split lines if this had been p << " " << "allocate" << "("; ? It also looks like p << " allocate("; is just fine here.
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.
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?
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.
I've tried to use that and it seemed to make the code more complex and so l decided not to use it.
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.
There is a mismatch in the order of arguments between the caller and the callee here.
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.
Nit question: Should this be renamed to verifyParallelOp?
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.
|86 ↗||(On Diff #294730)|
This unit-tests was removed in https://reviews.llvm.org/rGbe1197c403b22291e35cbc5e96788860ceabd40c