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

Mehdi AMINI via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Mar 22 13:20:02 PDT 2021


mehdi_amini added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp:248
+      op->erase();
+    opsToErase.clear();
+  }
----------------
kiranchandramohan wrote:
> 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);
>   }
> ```
>   
Yes this isn't more involved that what you're mentioning. I'll be happy to help with this kind of things, I just don't know how to proceed at the moment.

The DCE refactoring is trivial, I'm more concerned about the testing problem with the pass and the inability to have IR tests upstream when there are issues.


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

https://reviews.llvm.org/D98063



More information about the llvm-commits mailing list