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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp:54
+
+/// Convert fir.embox to the extended form where necessary.
+class EmboxConversion : public mlir::OpRewritePattern<EmboxOp> {
----------------
Is the notion of "extended form" for embox documented anywhere? If not can you expand the doc here to describe what it is?
(`rewriteStaticShape` and `rewriteDynamicShape` aren't documented, but that may not be necessary with a longer description for the pattern here, ideally with snippets example)

(same for all patterns)


================
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()) {
----------------
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)


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


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


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


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



================
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",
----------------
Why this change?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98063