[PATCH] D95399: [flang][fir] Upstream FIR dialect changes.

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

Thanks! It is exciting to start seeing the FIR change coming upstream!

The patch is not easy to review right now though (also it does not build), it seems like mixing a lot of unrelated things and it makes it not easy to understand what is tested and how. It would be much easier to review the canonicalization pattern change with its own test in isolation. Similarly for the addition of new ops, and the inliner interface for example. There is a non trivial amount of hand-written verifier diagnostics that don't seem tested either?



================
Comment at: flang/include/flang/Optimizer/Dialect/FIRAttr.h:137
+/// underlying type of the pointee object is left up to the client. Opaque
+/// attributes are always constructed as null pointers when parsing.
+class OpaqueAttr
----------------
I'm not sure how it works? How is the context reconstructed on reparsing these?

Also I don't see any test using such attribute right now, can you provide one?


================
Comment at: flang/include/flang/Optimizer/Dialect/FIRDialect.h:49
+  explicit FIRCodeGenDialect(mlir::MLIRContext *ctx);
+  virtual ~FIRCodeGenDialect();
+
----------------
I don't find a definition for these methods right now?


================
Comment at: flang/include/flang/Optimizer/Dialect/FIRDialect.h:68
   // clang-format on
+  registry.loadAll(&ctx);
 }
----------------
Why is this needed? In general this indicates a misconfiguration of the pipeline and shouldn't be used.

(also this method is not used in this patch)


================
Comment at: flang/include/flang/Optimizer/Dialect/FIROps.td:1107
+  let assemblyFormat = [{
+    $memref (`(` $shape^ `)`)? (`[` $slice^ `]`)? (`typeparams` $lenParams^)? (`map` $accessMap^)? attr-dict `:` functional-type(operands, results)
   }];
----------------
It'd be nice to have at least one test that parses all the fields as an example, and have it in the description here in ODS as well.


================
Comment at: flang/include/flang/Optimizer/Dialect/FIROps.td:1149
     }
     return mlir::success();
   }];
----------------
That's quite some C++ code in ODS. In general for non trivial method please replace with a call to a function implemented in the .cpp file: it plays much nicer with IDEs and debuggers.
(same below for the new ops you're introducing)


================
Comment at: flang/include/flang/Optimizer/Dialect/FIROps.td:1488
+      %d = fir.shape(%c_100) : (index) -> !fir.shape<1>
+      %b = fir.embox %r shape %d : (!fir.ref<i64>, !fir.shape<1>) -> !fir.box<i64>
       %a = fir.box_isarray %b : (!fir.box<i64>) -> i1  // true
----------------
Is the "shape" keyword a valid syntax? I'm not sure I understand from the declarative syntax above



================
Comment at: flang/include/flang/Optimizer/Dialect/FIROps.td:1945
+            fir::isa_char_string(eleTy)))
+        return emitOpError("cannot apply coordinate_of to this type");
     }
----------------
Are there tests for all these diagnostics?


================
Comment at: flang/lib/Optimizer/Dialect/FIROps.cpp:135
+    }
+  return result;
+}
----------------
This method is a bit strange: it returns `result` here but I don't see it ever modified?

If you only ever returns ranges of Value held by an operation, then `ValueRange` seems appropriate instead of `std::vector<Value>`


================
Comment at: flang/lib/Optimizer/Dialect/FIROps.cpp:467
     if (fir::GlobalOp::verifyValidLinkage(linkage))
-      return failure();
+      return mlir::failure();
     mlir::StringAttr linkAttr = builder.getStringAttr(linkage);
----------------
It isn't clear why this is needed?


================
Comment at: flang/lib/Optimizer/Dialect/FIROps.cpp:590
+// These patterns are written by hand because the tablegen pattern language
+// isn't adequate here.
+template <typename FltOp, typename CpxOp>
----------------
I'd rather read a comment that just describes what the pattern is doing on a high level / a snippet of the replacement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95399/new/

https://reviews.llvm.org/D95399