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

[Tom Eccles via Phabricator via cfe-commits]

tblah added inline comments.


================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:42
+  /// resulting FIR
+  EmitHLFIRToFIR,
+
----------------
awarzynski wrote:
> To me having `EmitFIR` and `EmitHLFIR` would make more sense. With 2 dialects, `EmitMLIR` becomes rather confusing (and, I suspect, rarely used).
EmitMLIR emits whichever one Lowering was configured for (depending on the -flang-experimental-hlfir flag).

EmitHLFIRToFIR always emits FIR after lowering via HLFIR and running all of the passes to convert HLFIR into FIR.

I didn't add EmitFIR and EmitHLFIR because the frontend action is exactly the same for these two (run lowering and print out the MLIR it generates).


================
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.");
----------------
awarzynski wrote:
> 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`).
This is very different to `EmitMLIR`.

`EmitMLIR` will print the MLIR produced by lowering (HLFIR or FIR depending upon the `-flang-experimental-hlfir` flag).

This action will run lowering, always generating HLFIR. Then it will run the HLFIR pass pipeline to lower the HLFIR into FIR, and output that FIR (which will be very different to FIR generated directly from lowering without going through HLFIR).


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:652
+void CodeGenAction::lowerHLFIRToFIR() {
+  assert(mlirModule && "The MLIR module has not been generated yet.");
+
----------------
awarzynski wrote:
> 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?
Yes. In this case, lowering will have always produced HLFIR because we don't enter this action unless `-flang-experimental-hlfir` has been specified.


================
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);
----------------
awarzynski wrote:
> "Lowering to LLVM IR"? ;-)
Thanks for spotting this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151088