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

[Eric Schweitz via Phabricator via llvm-commits]

schweitz added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp:132
+    llvm::SmallVector<mlir::Value, 8> shapeOpers;
+    llvm::SmallVector<mlir::Value, 8> shiftOpers;
+    if (auto shapeVal = rebox.shape()) {
----------------
mehdi_amini wrote:
> Nit: you can omit the `, 8` everywhere, `SmallVector` now computes a default.
> 
> (that is unless you know that 8 is better than the default in this particular case of course)
Thanks.


================
Comment at: flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp:199
+
+/// Convert FIR structured control flow ops to CFG ops.
+class CodeGenRewrite : public CodeGenRewriteBase<CodeGenRewrite> {
----------------
mehdi_amini wrote:
> Is this comment up-to-date?
Removed.


================
Comment at: flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp:234
+      runOn(global, global.getRegion());
+  }
+
----------------
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.


================
Comment at: flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp:240
+      for (auto &op : block.getOperations()) {
+        if (op.getNumRegions() != 0)
+          for (auto &reg : op.getRegions())
----------------
mehdi_amini wrote:
> This test looks spurious? The loop inside the body would not execute if there are no region right?
Removed.


================
Comment at: flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp:248
+      op->erase();
+    opsToErase.clear();
+  }
----------------
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.


================
Comment at: flang/tools/fir-opt/fir-opt.cpp:22
   fir::support::registerFIRPasses();
+  fir::registerOptPasses();
   DialectRegistry registry;
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > Why this change?
> What is the intended difference between `registerFIRPasses` and `registerOptPasses` ? 
Fixed.


================
Comment at: flang/tools/fir-opt/fir-opt.cpp:22
   DialectRegistry registry;
-  fir::support::registerDialects(registry);
   return failed(MlirOptMain(argc, argv, "FIR modular optimizer driver\n",
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98063