[PATCH] D82693: [flang] Upstream two FIR transformation passes

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D82693#2119807 <https://reviews.llvm.org/D82693#2119807>, @richard.barton.arm wrote:

> Stupid question perhaps, but if they are two passes, shouldn't they be added in two commits?


In the passes files there are 6 pass creators declared. I failed to find all the definitions in this patch.
It seems sensible to split this into multiple commits. I would also recommend tests for new functionality.

[EDIT: Took too long to write my comments, seems abandoned now]



================
Comment at: flang/include/flang/Optimizer/Transforms/Passes.td:22
+  let description = [{
+    TODO
+  }];
----------------
sscalpone wrote:
> Is the a pro forma description that could be added?
Yes, please.


================
Comment at: flang/lib/Optimizer/Transforms/Inliner.cpp:35
+    return mlir::createInlinerPass();
+  llvm::report_fatal_error("inlining is disabled");
+}
----------------
I would suggest to make the inliner pass a no-op if it is disabled.
The CFG conversion pass works that way already (I think).

I can also see that the enable flag will prevent it from being put in the pipeline but disabling the create function here with a comment suggesting there is a hidden dependence to another function seems fragile. It is also counter-intuitive to people that have seen `llvm::createXXXPass` functions before.


================
Comment at: flang/lib/Optimizer/Transforms/RewriteLoop.cpp:84
+    if (onetrip)
+      rewriter.create<mlir::BranchOp>(loc, firstBlock, ArrayRef<mlir::Value>());
+    else
----------------
There seems little reason to commit dead code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82693