[PATCH] D98063: [flang][fir] Add the pre-code gen rewrite pass and codegen ops.

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

(seems like you need to run `git clang-format`)



================
Comment at: flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp:234
+      runOn(global, global.getRegion());
+  }
+
----------------
schweitz wrote:
> mehdi_amini wrote:
> > By making this a module pass, we're losing on parallelism.
> > Can you make is an operation pass and filter here instead?
> > 
> > ```
> > void runOnOperation()  {
> >   Operation *op = getOperation();
> >   if (auto func = op->dyn_cast<mlir::FuncOp>()) runOn(func, func.getBody());
> >   if (auto global : op->dyn_cast<fir::GlobalOp>()) runOn(global, global.getRegion());
> > }
> > ```
> It might be possible, but a quick experiment showed a bunch of tests regressing.
What kind of regressions?


================
Comment at: flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp:248
+      op->erase();
+    opsToErase.clear();
+  }
----------------
schweitz wrote:
> mehdi_amini wrote:
> > I don't quite get the delayed erasing, this whole simplification could be done without keeping state in a vector:
> > 
> > region.walk([] (Operation *op) {
> >   if (auto embox = dyn_cast<EmboxOp>(op)) {
> >       if (embox.getShape()) {
> >         op->erase(); 
> >         return;
> >      }
> >   }
> > }
> > ```
> > 
> > (note that `op->erase()` should already assert for `op->use_empty()`, no need to duplicate here)
> > 
> > But also, could you do it more simply:
> > 
> > ```
> > region.walk([] (Operation *op) {
> >   if (isOpTriviallyDead(op)) op->erase();
> > }
> > ```
> > ?
> > 
> This should be a very general DCE like in mlir Transforms/.CSE.cpp. That seems to have been mangled but can be brought back.
Can you just call this instead? https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Transforms/RegionUtils.h#L57

In general generic algorithm in passes like this ends up being replicated everywhere when they can be exposed a general utilities, if the `simplifyRegions` does not fit the bill here can we introduce another utility there?


================
Comment at: flang/tools/fir-opt/fir-opt.cpp:25
   fir::support::registerDialects(registry);
+  registry.insert<fir::FIROpsDialect>();
   return failed(MlirOptMain(argc, argv, "FIR modular optimizer driver\n",
----------------
This does not seem needed


================
Comment at: flang/tools/fir-opt/fir-opt.cpp:22
   fir::support::registerFIRPasses();
+  fir::registerOptPasses();
   DialectRegistry registry;
----------------
schweitz wrote:
> schweitz wrote:
> > mehdi_amini wrote:
> > > mehdi_amini wrote:
> > > > Why this change?
> > > What is the intended difference between `registerFIRPasses` and `registerOptPasses` ? 
> > Fixed.
> A synch problem with the source.
It is still unclear to me what is the intent here: `registerOptimizerPasses` is not documented, and `registerMLIRPassesForFortranTools` says "Register the standard passes we use" which seems like it should have all the required passes.

Why do you introduce `registerOptimizerPasses` at all?


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

https://reviews.llvm.org/D98063