[PATCH] D151088: [flang][hlfir] Separate -emit-fir and -emit-hlfir for flang-new

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks Tom, mostly makes sense, but I suggest the following:

- `EmitMLIR` --> `EmitFIR` (nfc, just clarifying the name),
- `EmitHLFIRToFIR` --> `EmitHLFIR` (functional change).

More comments inline.



================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:42
+  /// resulting FIR
+  EmitHLFIRToFIR,
+
----------------
To me having `EmitFIR` and `EmitHLFIR` would make more sense. With 2 dialects, `EmitMLIR` becomes rather confusing (and, I suspect, rarely used).


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:651
+// Lower using HLFIR then run the FIR to HLFIR pipeline
+void CodeGenAction::lowerHLFIRToFIR() {
+  assert(mlirModule && "The MLIR module has not been generated yet.");
----------------
I wouldn't really consider this hook as a separate action. Instead, I'd use it here: https://github.com/llvm/llvm-project/blob/6130c9df99a7a7eb9c6adc118a48f8f2acc534ab/flang/lib/Frontend/FrontendActions.cpp#L917-L920. As in, it basically "tweaks" `EmitMLIR` (which I would rename as `EmitFIR`).


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:652
+void CodeGenAction::lowerHLFIRToFIR() {
+  assert(mlirModule && "The MLIR module has not been generated yet.");
+
----------------
This `mlirModule` comes from here: https://github.com/llvm/llvm-project/blob/6130c9df99a7a7eb9c6adc118a48f8f2acc534ab/flang/lib/Frontend/FrontendActions.cpp#L277. That will either be FIR or HLFIR, depending on whether `-flang-experimental-hlfir` was used or not, right?


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:673
+    unsigned diagID = ci.getDiagnostics().getCustomDiagID(
+        clang::DiagnosticsEngine::Error, "Lowering to LLVM IR failed");
+    ci.getDiagnostics().Report(diagID);
----------------
"Lowering to LLVM IR"? ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151088