[PATCH] D111337: [fir] Add array value copy pass

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:355
+  auto boxTy = fir::BoxType::get(elementType);
+  return exv.match(
+      [&](const fir::ArrayBoxValue &box) -> mlir::Value {
----------------
mehdi_amini wrote:
> I don't quite find what the `.match(` call resolves to on the `fir::ExtendedValue` class? Any tip?
https://github.com/llvm/llvm-project/blob/102d2a8a99057b2f54ca97661f862e33d055171a/flang/include/flang/Optimizer/Builder/BoxValue.h#L406


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:453-462
+          if (useMap.count(acc)) {
+            mlir::emitError(
+                load.getLoc(),
+                "The parallel semantics of multiple array_merge_stores per "
+                "array_load are not supported.");
+            return;
+          }
----------------
mehdi_amini wrote:
> I commented on this before I believe: you have two map lookup when one would be enough I think:
> 
Yeah sorry I thought I made the update. Must have been lost in an other branch. 


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:644
+                        ArrayOp update,
+                        const std::function<void(mlir::Value)> &assignElement,
+                        mlir::Type lhsEltRefType) const {
----------------
mehdi_amini wrote:
> 
Sorry missed this update as well. It's updated. 


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:716
+              fir::getKindMapping(update->getParentOfType<mlir::ModuleOp>()));
+          if (!update.typeparams().empty()) {
+            auto boxTy = fir::BoxType::get(inEleTy);
----------------
mehdi_amini wrote:
> This branch may be worth a comment.
> 
> In general the entire pattern is a bit light on documentation.
`ArrayUpdateOp` is actually not supported for reference type anymore since there is a new op for it. This code is removed. 


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:730
+          llvm::report_fatal_error("not a legal reference type");
+        }
+      } else {
----------------
mehdi_amini wrote:
> You can reduce the indentation in the code by inverting the else and the then branch:
> 
> ```
>         if (!inEleTy.isa<fir::RecordType>())
>            llvm::report_fatal_error("not a legal reference type");
> ```
> 
> Also, I'm not sure why it isn't an assertion here? If this can't be then shouldn't we degrade more gracefully? 
> ```
> assert(inEleTy.isa<fir::RecordType>());
> ```
Replaced with an assert and remove the if-else


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:120
+
+  void collectArrayAccessFrom(mlir::Operation *op, mlir::Value val) {
+    // `val` is defined by an Op, process the defining Op.
----------------
kiranchandramohan wrote:
> clementval wrote:
> > mehdi_amini wrote:
> > > clementval wrote:
> > > > mehdi_amini wrote:
> > > > > I am very concerned about a recursive algorithm on def-use chain: this is a ticking bomb for a stack overflow in the compiler.
> > > > > Can you make this an iterative algorithm with an explicit stack?
> > > > Would it be ok to address this in a follow up patch? I just added a todo right now. 
> > > How to we track that this get addressed though? I'm wary of TODO that will stay here for a while. 
> > > Is someone gonna commit to work on this ASAP? 
> > I can work on that in the next 2-3 weeks probably. 
> I have created an issue to capture this in the upstreaming FIR Passes board. 
> https://github.com/flang-compiler/f18-llvm-project/projects/7
> https://github.com/flang-compiler/f18-llvm-project/issues/1268
> 
> @clementval if this is just about converting to an iterative version then we can help.
Yeah this is it. With all the changes (compare to fir-dev) in this patch it would be nice to be able to make a sync before this kind of refactoring. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111337