[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:

Option 2 was something I briefly investigated some time ago, but I think it isn't something realistic for us to do right now. For instance, `omp.map.info` ops can be chained (their values being passed as operands to other `omp.map.info`s) to represent structures. We would completely have to rework how we represent this relationship, and then it might be possible to eventually get there, but it would take very significant effort, easily introduce regressions and bugs in a notoriously difficult to troubleshoot part of the compiler and the MLIR representation for the resulting ops would be augmented by a significant amount. Back when I looked into it, that last point was the main blocker, as we didn't have clause-based representations and it would have been incredibly difficult to keep `map` representation consistent across operations over time. Now, I think the main problem is that mapping support has matured quite a bit and there is a lot of stuff built on top of this early design decision.

We could indeed remove user-specific checks, which are mostly about which map types are allowed for which OpenMP directives. This should be checked by frontends anyways, but I think it also makes sense for MLIR to verify that invariant, as followup translation code won't be prepared to deal with unexpected map types. I think it would just be unfortunate to lose the verification of that invariant just to fit better the mold, since at the end of the day those `omp.map.info` ops aren't that independent: they can only be used as specific operands to specific ops in the OpenMP dialect (and I'm aware this already points at a bad design to start with).

In my opinion, the current representation is a compromise. It's not great as a representation for an MLIR op, since it probably makes more sense as a set of operands/attributes attached to every operation taking a map-like OpenMP clause. But it also helps with readability and there's already a lot built on top of it. Transitioning to something else would have to be a long term goal, but I don't think removing part of the verification logic will get us closer, so I'd personally prefer to keep it until then. Probably @agozillon has a more complete view on this topic and can chime in as well.

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