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

Fri Nov 26 04:21:35 PST 2021

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.

In D111337#3154805 <https://reviews.llvm.org/D111337#3154805>, @mehdi_amini wrote:

> @kiranchandramohan do you plan on reviewing/approving the current version of the patch?
>
> I don't have any more comment than before, there are just a few that are still pending.

The patch LGTM. Thanks for going through the patch in detail and pointing out various issues. And thanks to @clementval for addressing most of the issues. I agree that the remaining issues can be dealt with later. I have created a few github issues to follow up on later work.

Nit: The changes in the following files are in other patches or have been merged. How are you planning to handle this?
flang/lib/Optimizer/Dialect/FIRType.cpp
flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h
flang/include/flang/Optimizer/Builder/Runtime/Assign.h
flang/lib/Optimizer/Builder/Runtime/Assign.cpp

================
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:
> clementval wrote:
> > 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
> Thanks!
> 
> Something really confusing is that there is another class with the same name, and located in a header with the same filename! 
> (in `include/flang/Lower/Support/BoxValue.h`)
I think there is some refactoring going on, where code in Lower is moved to the Optimizer/Builder directory. I think once all the code is moved the code in Lower can be deleted.

================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:30
+                                      "name"),
+                       llvm::cl::init(32));
+
----------------
clementval wrote:
> mehdi_amini wrote:
> > mehdi_amini wrote:
> > > I'm wary of global options in general (Can we build the optimizer with `-Werror=global-constructors` by the way?), what is this used for? Can you find an alternative (like store it on the dialect for example)?
> > This is marked done, but you actually extracted it as-is and landed it in https://reviews.llvm.org/D112057 ; can you address that as well (in a separate patch, but I'm wary of losing track of this).
> > 
> Sorry, I marked it as done here to be able to navigate more easily in the comments since it was not part of this part anymore. I'll look at it in another patch. 
Raised an issue for this https://github.com/flang-compiler/f18-llvm-project/issues/1273

================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:46
+                                                llvm::StringRef name) {
+  return modOp.lookupSymbol<fir::GlobalOp>(name);
+}
----------------
clementval wrote:
> mehdi_amini wrote:
> > mehdi_amini wrote:
> > > Do these one-line functions carry their weight? 
> > > Seems like just a renaming that kind of obfuscate what's happening (I'd have to jump through more hoops in the IDE to figure it out).
> > Same here: you marked it done without commenting and landed it separately.
> Is this really important? If so I'll update it. 
Might be easier to read for newcomers.

================
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.
----------------
clementval wrote:
> 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. 
OK. I have moved this to the post-upstream board.

================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:309
+        auto arg = blockArg.getArgNumber();
+        auto output = ro.getResult(ro.finalValue() ? arg : arg - 1);
+        appendToQueue(output);
----------------
clementval wrote:
> mehdi_amini wrote:
> > mehdi_amini wrote:
> > > The `finalValue()` check here is also another case not covered in the test.
> > > 
> > > `finalValue` isn't documented in ODS and `initArgs` isn't documented either, it'd be great if this could be addressed (separately from this revision of course)
> > > 
> > 
> They are loops with `finalValue` and `iterArgs` in the tests. Will add the documentation in another path. Added on my todo list. 
Raised https://github.com/flang-compiler/f18-llvm-project/issues/1274

================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:315-319
+      for (auto i : llvm::enumerate(operands))
+        if (operand->get() == i.value()) {
+          auto blockArg = dest->getArgument(i.index());
+          appendToQueue(blockArg);
+        }
----------------
clementval wrote:
> mehdi_amini wrote:
> > This loop isn't great when you already have everything to compute offsets.
> > 
> > 
> > In general all the special casing for the Loop, branches, and the control flow in general should be handled with the ControlFlowInterface: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
> > 
> > This would make the code entirely uniform, and any new op that has to do with the control flow would not require to modify the pass.
> > 
> > I'm fine if you want to handle this later for this pass (please add a big `// TODO: this need to be updated to use the control-flow interface`), but I'm not sure we should upstream many other passes that would be implemented without going through interfaces.
> > 
> I think it is better to handle this later (switch to control flow interface) as it will bring lots of changes with it. 
Raised https://github.com/flang-compiler/f18-llvm-project/issues/1275 to look into this later.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111337