[PATCH] D85508: [flang] Add rudimentary empty function lowering to tco

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

We really need passes to be tested individually with an `opt` like tool, let me know if you need more pointers.
I suspect you also should invest up-front in the same registration/description for passes as in the mlir core folder.



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1
+#include "flang/Optimizer/CodeGen/CodeGen.h"
+#include "mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h"
----------------
There should be a license header here I believe?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:18
+static cl::opt<bool> disableLLVM("disable-llvm", cl::desc("disable LLVM pass"),
+                                 cl::init(false), cl::Hidden);
+
----------------
This kind of option is unusual: if you want to disable passes this is better done at the point where the pass pipeline is built.

(Also please use PassOptions instead of globals)


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:26
+// Lower a SELECT operation into a cascade of conditional branches. The
+// last case must be the `true` condition.
+/// Convert FIR dialect to LLVM dialect
----------------
This comment seems misplaced


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:62
+private:
+  fir::NameUniquer &uniquer;
+};
----------------
In general passes aren't supposed to depends on external entities like this: they are expected to be registered and invoked from an *-opt tool.

Here isn't clear why it can't just be a private instance for the pass?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85508