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

Mon Nov 22 13:05:14 PST 2021

clementval marked 51 inline comments as done.
clementval added a comment.

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

> Thanks for the update, the size makes it tractable now, and the test helps to figure out the corner cases when the code isn't straightforward.
>
> I mostly looked at `ArrayCopyAnalysis` here.
>
> One thing that can be problematic for future patches: 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> which may not be yet implemented on the relevant FIR ops. But that had a significant impact on the code inside the passes, so likely something to consider prioritizing downstream.

Tried to address most of the comments. I think we will need couple of follow up patches to make it easier to move foward (and sync back in between).

================
Comment at: flang/include/flang/Optimizer/Builder/FIRBuilder.h:403
+fir::ExtendedValue readBoxValue(fir::FirOpBuilder &builder, mlir::Location loc,
+                                const fir::BoxValue &box);
+
----------------
mehdi_amini wrote:
> Is there a use for this method? (same for the other above)
This one is not used currently (removed from this patch). The one above is. 

================
Comment at: flang/include/flang/Optimizer/Builder/FIRBuilder.h:417
+fir::ExtendedValue createStringLiteral(fir::FirOpBuilder &, mlir::Location,
+                                       llvm::StringRef string);
+
----------------
mehdi_amini wrote:
> No definition added for this declaration?
Mistake of the rebase. It's the same as the one just above. Removed. 

================
Comment at: flang/include/flang/Optimizer/Transforms/Factory.h:145
+/// \p shapeVal is empty or is a fir.shift.
+inline std::vector<mlir::Value> getExtents(mlir::Value shapeVal) {
+  if (shapeVal)
----------------
mehdi_amini wrote:
> Do you know why this is returning a vector instead of an OperandRange? (this is a question for the `ShapeOp` definition).
> 
> It seems strange to force copy on all the accessors when they may just want to iterate on the Values...
I don't think there is a good reason for this. Will add a TODO so we can address this in a follow up patch since it touches the op definiton and probably other places in the code. 

================
Comment at: flang/include/flang/Optimizer/Transforms/Passes.td:95
+  let dependentDialects = [
+    "fir::FIROpsDialect", "mlir::StandardOpsDialect"
+  ];
----------------
mehdi_amini wrote:
> Can you just let me know what fails if you remove any of these?
These were artifact from the time it didn't work without it. Looks like it is now fine to remove them. 

================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:289
+  //   return createConvert(loc, toTy, rp);
+  // }
+  return createConvert(loc, toTy, val);
----------------
mehdi_amini wrote:
> Why the commented out code? Can it just be removed?
Yeah will be added later anyway. 

================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:89
+  /// Get all the array value operations that use the original array value
+  /// as passed to `store`.
+  void arrayAccesses(llvm::SmallVectorImpl<mlir::Operation *> &accesses,
----------------
mehdi_amini wrote:
> I don't understand this comment (I read it 4 times though).
> 
> Edit: the comment on the implementation is actually much more clear: 
> 
> ```
> /// Find all the array operations that access the array value that is loaded by
> /// the array load operation, `load`.
> ```
> 
> 
Updated.

================
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.
----------------
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. 

================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:155
+      return;
+    }
+
----------------
mehdi_amini wrote:
> Of all the `if` above, only the `DoLoopOp` seems tested. There are some specific cases for the treatment of EmboxOp and ArrayMergeStoreOp that likely should be covered
Added some test and removed the `IterWhileOp` and `EmboxOp` special case since they will be added later when we can add better test coverage. 

================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:234
+    if (auto doLoop =
+            mlir::dyn_cast_or_null<fir::DoLoopOp>(seq.getDefiningOp()))
+      loopRegion = &doLoop->getRegion(0);
----------------
mehdi_amini wrote:
> Should this also support other kind of loops? (if so add a TODO)
>From a practical point of view, array operations are normally generated with `DoLoopOp` so there is no real code that need the other loops. From a purely FIR point of view, of course someone could write some `IterWhileOp` with array operations but we do not have use cases in mind for that. 
I added a comment to make precise this in the code. 

================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:260
+    for (auto indirectUser : op->getUsers())
+      followUsers(indirectUser);
+  }
----------------
mehdi_amini wrote:
> Same recursion issue here.
Same here. Would be great if we can address that in a follow up patch directly after finishing this one. 

================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:309
+        auto arg = blockArg.getArgNumber();
+        auto output = ro.getResult(ro.finalValue() ? arg : arg - 1);
+        appendToQueue(output);
----------------
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. 

================
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);
+        }
----------------
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. 

================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:360
+  }
+  loadMapSets.insert({load, visited});
+}
----------------
mehdi_amini wrote:
> Seems like we're storing much more than what we need actually, I tried locally to change `loadMapSets` to store the SmallVector of "accesses" and it is just enough (and it makes subsequent call even easier).
> 
> Here is a patch: https://gist.github.com/joker-eph/8f00e79783688bf6349b6b5d9d44a480
> 
Thanks for the patch. look like it does the trick.

================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:438-442
+  for (auto &region : regions)
+    for (auto &block : region.getBlocks())
+      for (auto &op : block.getOperations()) {
+        if (op.getNumRegions())
+          construct(op.getRegions());
----------------
mehdi_amini wrote:
> Isn't this entire loop nest + recursion just a walk?
> 
> 
> Also, I'd change the signature of the `construct` method to be:
> `void ArrayCopyAnalysis::construct(Operation *topLevelOp) {`
> 
> So that you can just write `topLevelOp->walk([&] (Operation *op) {`
> 
> (the only reason for the current signature was the recursion as far as I can tell)
> 
Thanks for the inputs here. Replace by a simple walk.

================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:448
+          arrayAccesses(accesses,
+                        mlir::cast<ArrayLoadOp>(st.original().getDefiningOp()));
+          if (conflictDetected(values, accesses, st)) {
----------------
mehdi_amini wrote:
> Where is the guarantee that this cast will always succeed coming from? 
> The doc for `ArrayMergeStoreOp` does not mention anything, and there is no verifier for `ArrayMergeStoreOp`
`ArrayMergeStoreOp` verifier has this:

```
if (!isa<ArrayLoadOp>(op.original().getDefiningOp()))
    return op.emitOpError("operand #0 must be result of a fir.array_load op");
```

================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:823
+    // Phase 1 is performing a rewrite on the array accesses. Once all the
+    // array accesses are rewritten we can go on pahse 2.
+    // Phase 2 gets rid of the useless copy-in/copyout operations. The copy-in
----------------
mehdi_amini wrote:
> (typo)
Thanks.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111337