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

[Eric Schweitz via Phabricator via llvm-commits]

schweitz 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);
+
----------------
mehdi_amini wrote:
> 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.
Your objections are noted. We'd like to table this for the moment so as to continue upstreaming.


================
Comment at: flang/include/flang/Optimizer/Support/InitFIR.h:13
+
+#ifndef OPTIMIZER_SUPPORT_INITFIR_H
+#define OPTIMIZER_SUPPORT_INITFIR_H
----------------
mehdi_amini wrote:
> Seems like all the header guards need an update.
This is unexpected as it follows the same pattern used in previously upstreamed headers. We'll figure out what changed in clang-tidy.


================
Comment at: flang/include/flang/Optimizer/Support/InitFIR.h:37
+                  mlir::StandardOpsDialect,
+                  mlir::vector::VectorDialect>();
+  // clang-format on
----------------
mehdi_amini wrote:
> There is no loading involved, the function should be renamed here (and the doc updated)
Right. The change to registration was made a day or two ago.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96454