[PATCH] D96454: [flang][fir] Update the code generation test tool

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: flang/include/flang/Optimizer/Support/FIRContext.h:32
+/// module `mod` is still live.
+void setTargetTriple(mlir::ModuleOp mod, llvm::Triple &triple);
+
----------------
I have concerns with such approach that makes the compilation flow very stateful, where things like a Triple can easily be encoded in the IR as an attribute for example.


================
Comment at: flang/include/flang/Optimizer/Support/InitFIR.h:13
+
+#ifndef OPTIMIZER_SUPPORT_INITFIR_H
+#define OPTIMIZER_SUPPORT_INITFIR_H
----------------
Seems like all the header guards need an update.


================
Comment at: flang/include/flang/Optimizer/Support/InitFIR.h:37
+                  mlir::StandardOpsDialect,
+                  mlir::vector::VectorDialect>();
+  // clang-format on
----------------
There is no loading involved, the function should be renamed here (and the doc updated)


================
Comment at: flang/include/flang/Optimizer/Support/InitFIR.h:42
+/// Register the standard passes we use. This comes from registerAllPasses(),
+/// but is a smaller set since we aren't using many of the passes found there.
+inline void registerGeneralPasses() {
----------------
Make a note that registration is only ever useful to be able to parse the passes from a textual string (mostly in testing tools) and not to construct a pass pipeline.


================
Comment at: flang/include/flang/Optimizer/Support/InitFIR.h:69
+
+inline void registerFIRPasses() { registerGeneralPasses(); }
+
----------------
Do we need two functions to do the same thing? Can you keep one?


================
Comment at: flang/lib/Optimizer/Support/FIRContext.cpp:23
+void fir::setTargetTriple(mlir::ModuleOp mod, llvm::Triple &triple) {
+  mod->setAttr(tripleName, fir::OpaqueAttr::get(mod.getContext(), &triple));
+}
----------------
Opaque attribute are supposed to be non-semantically impactful I believe and be able to be dropped, Triple does not seems to fit here.

I actually don't know about uniquer and kindmap, but this is making me nervous as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96454