[Mlir-commits] [mlir] [MLIR][ODS] Require the value after oilist keyword (PR #229334)

[Sergio Afonso]

================
@@ -3002,8 +3002,16 @@ func.func @omp_target_update_invalid_motion_type(%map1 : memref<?xi32>) {
 
 // -----
 
-func.func @omp_target_map_must_specify_both_var_ptr_ptr_args(%arg : memref<?xi32>) {
+func.func @omp_map_var_ptr_ptr_missing_type(%arg : memref<?xi32>) {
+  // expected-error @below {{expected attribute value}}
   %map1 = omp.map.info var_ptr(%arg : memref<?xi32>, tensor<?xi32>) map_clauses(to) capture(ByRef) var_ptr_ptr(%arg : memref<?xi32>, ) name("") -> memref<?xi32>
+  return
+}
+
+// -----
+
+func.func @omp_target_map_must_specify_both_var_ptr_ptr_args(%arg : memref<?xi32>) {
+  %map1 = "omp.map.info"(%arg, %arg) <{map_capture_type = #omp.variable_capture_kind<ByRef>, map_type = #omp.clause_map_flags<to>, operandSegmentSizes = array<i32: 1, 1, 0, 0>, var_ptr_type = tensor<?xi32>}> : (memref<?xi32>, memref<?xi32>) -> memref<?xi32>
----------------
skatrak wrote:

> Oh I see, this looks like the OpenMP dialect is verifying the `omp.map.info` in the verifier of the user of the result of the omp.map.info, this is against MLIR verifier guidelines.

Yes, it has been this way for a long time and it's not great. The problem is that whether an `omp.map.info` is valid depends on which OpenMP operation(s) it's attached to. The verification logic itself is centralized, but it's accessed by the verifier of every user of `omp.map.info` ops rather than the operation itself. We could probably reverse these checks and base them on iterating across `omp.map.info` users instead, though.

https://github.com/llvm/llvm-project/pull/229334