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

[Eric Schweitz via Phabricator via llvm-commits]

schweitz marked 2 inline comments as done.
schweitz added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp:234
+      runOn(global, global.getRegion());
+  }
+
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > 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?
> Actually in case I wasn't clear, the snippet I wrote above requires to schedule the pass twice in a pipeline, the test would look like this: `fir-opt --pass-pipeline="func(cg-rewrite),fir.global(cg-rewrite)"`
> 
> If you can provide an .mlir test where this regresses, I'd be happy to take a look.
I spent some time working to make this pass multithreaded, including converting data structures to be thread local, etc. We have approximately 800 basic tests and many thousands of Fortran tests. The attempted changes caused many of these tests to regress and start failing. The existing pass accomplishes its task as is. There is no data-based argument that the pass is a bottleneck. The code is being upstreamed in a series of many patches, and there will be ample opportunity to write and debug a parallel algorithm in subsequent patches. Finally, anyone that wants to can contribute improvements on fir-dev.


================
Comment at: flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp:248
+      op->erase();
+    opsToErase.clear();
+  }
----------------
mehdi_amini wrote:
> 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?
Possibly. For the time being, we prefer to keep the simple dead-code cleanup.


================
Comment at: flang/tools/fir-opt/fir-opt.cpp:22
   fir::support::registerFIRPasses();
+  fir::registerOptPasses();
   DialectRegistry registry;
----------------
mehdi_amini wrote:
> 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?
Upstreaming the code is being done in a series of many patches. I have chosen to group code in ways that make the upstreaming process more manageable. For example, the MLIR registration interfaces have been changing, and we want those calls isolated until they can be upstreamed. Furthermore, link times are already expensive, so we have made efforts for reduce that impact by selecting specific required libraries from MLIR. For these reasons there is no reason to change this now. There will be opportunities to regroup registration calls in subsequent patches


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

https://reviews.llvm.org/D98063