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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp:248
+      op->erase();
+    opsToErase.clear();
+  }
----------------
mehdi_amini wrote:
> schweitz wrote:
> > 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.
> I'm fine if you don't want to call `simplifyRegions`, but then can you refactor this as a pre-patch into a utility in a common place to avoid code duplication then?
> 
> I'm not enthusiastic to see each pass reimplementing generic utility / algorithm, and I don't see really a reason why we should at all.
> 
> This has community/project wise implications as well, the CIRCT folks were reporting performance issues with this kind of code path just a few days ago: https://llvm.discourse.group/t/speeding-up-canonicalize/3015 ; we were able to work together to improve the general tooling and infra around this.
Is the suggestion here to move the following DCE code to a file inside the mlir directory tree?

```
// Clean up the region.
  void simplifyRegion(mlir::Region &region) {
    for (auto &block : region.getBlocks())
      for (auto &op : block.getOperations()) {
        for (auto &reg : op.getRegions())
          simplifyRegion(reg);
        maybeEraseOp(&op);
      }
    doDCE();
  }

  /// Run a simple DCE cleanup to remove any dead code after the rewrites.
  void doDCE() {
    std::vector<mlir::Operation *> workList;
    workList.swap(opsToErase);
    while (!workList.empty()) {
      for (auto *op : workList) {
        std::vector<mlir::Value> opOperands(op->operand_begin(),
                                            op->operand_end());
        LLVM_DEBUG(llvm::dbgs() << "DCE on " << *op << '\n');
        ++numDCE;
        op->erase();
        for (auto opnd : opOperands)
          maybeEraseOp(opnd.getDefiningOp());
      }
      workList.clear();
      workList.swap(opsToErase);
    }
  }

  void maybeEraseOp(mlir::Operation *op) {
    if (!op)
      return;
    if (op->hasTrait<mlir::OpTrait::IsTerminator>())
      return;
    if (mlir::isOpTriviallyDead(op))
      opsToErase.push_back(op);
  }
```
  


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

https://reviews.llvm.org/D98063