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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp:248
+      op->erase();
+    opsToErase.clear();
+  }
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > schweitz wrote:
> > > mehdi_amini wrote:
> > > > 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.
> > > The original suggestion was to make this pass not work on ModuleOp so that it would allow multithreading. That suggestion did not contain enough information. The code snippet does not compile. Fixing the compilation led to the test included in this patch failing. Subsequently an incorrect guess was made that appeared to fix the test in this patch. However, the guessed at solution was wrong as other tests not merged regressed. This was all done in an effort to follow your original suggestion.
> > > 
> > > Clearly it is just not possible to merge tests that require code that has not been merged. 
> > I am not sure what you're trying to say here, would you like me to send a patch that can be applied on top of this one and change this pass in a way that works with the test in-tree?
> Here is the patch: https://reviews.llvm.org/D99132 for this part of the discussion.
Seems like you updated to use this version of the patch. I'm sorry it took so long to get there for this revision: if something isn't clear with any of my comment feel free to ask me to clarify and I'm always happy to send a patch when it helps!


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

https://reviews.llvm.org/D98063