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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

Looks fine overall, thanks!

There are still a few minor issues: dead code in particular, but also the code documentation is a bit on the light side for something that really does not seem trivial to me.
(+ some inline comments)



================
Comment at: flang/include/flang/Optimizer/Builder/FIRBuilder.h:66
+  /// value to have the proper semantics and be strongly typed.
+  mlir::Value convertWithSemantics(mlir::Location loc, mlir::Type toTy,
+                                   mlir::Value val);
----------------
Seems unused in this patch?


================
Comment at: flang/include/flang/Optimizer/Builder/FIRBuilder.h:350
+                                   mlir::Type type);
+
 private:
----------------
Unused in this patch?


================
Comment at: flang/include/flang/Optimizer/Builder/FIRBuilder.h:423
+                                            mlir::Location loc,
+                                            mlir::Value component);
+
----------------
This is unused in this patch?



================
Comment at: flang/include/flang/Optimizer/Builder/FIRBuilder.h:434
+                                               const fir::ExtendedValue &array,
+                                               mlir::Value element);
+
----------------
Same


================
Comment at: flang/include/flang/Optimizer/Builder/FIRBuilder.h:443
+
+mlir::TupleType getRaggedArrayHeaderType(fir::FirOpBuilder &builder);
+
----------------
Unused as well (and undocumented)


================
Comment at: flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h:359
+                                  AsSequence(X)>
+#define mkRTKey(X) mkKey(RTNAME(X))
+
----------------
Can you document all this macro system?
Does it need to in header? In general macros defined in header is "dangerous", should these be prefixed with Fir or something?


================
Comment at: flang/include/flang/Optimizer/Builder/Runtime/RTBuilder.h:363
+/// argument is intended to be of the form: <mkRTKey(runtime function name)>
+/// Clients should add "using namespace Fortran::runtime"
+/// in order to use this function.
----------------
You could add this in the function body if this is really needed, or what do I misse?


================
Comment at: flang/include/flang/Optimizer/Transforms/Factory.h:44
+void genCharacterCopy(mlir::Value src, mlir::Value srcLen, mlir::Value dst,
+                      mlir::Value dstLen, B &builder, mlir::Location loc) {
+  auto srcTy =
----------------
This method isn't used anywhere either


================
Comment at: flang/include/flang/Optimizer/Transforms/Factory.h:147
+/// `std::vector<mlir::Value>` to avoid copies.
+inline std::vector<mlir::Value> getExtents(mlir::Value shapeVal) {
+  if (shapeVal)
----------------
I don't see a call-site for this either


================
Comment at: flang/include/flang/Optimizer/Transforms/Factory.h:190-192
+      assert(dimension == mlir::cast<fir::ShapeOp>(shapeVal.getDefiningOp())
+                              .getType()
+                              .getRank());
----------------
Move this assert with the one a few lines above? They seem related.
Using the `assert(!shapeVal ||` form like above you can avoid the `if` and the nesting


================
Comment at: flang/include/flang/Optimizer/Transforms/Factory.h:253
+      llvm::make_range(steps.begin(), steps.end()), threadedVals, unordered);
+}
+
----------------
The createLoopNest functions seem unused as well


================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:347
+                                         const fir::ExtendedValue &exv) {
+  auto itemAddr = fir::getBase(exv);
+  if (itemAddr.getType().isa<fir::BoxType>())
----------------



================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:353
+    mlir::emitError(loc, "internal: expected a memory reference type ")
+        << itemAddr.getType();
+  auto boxTy = fir::BoxType::get(elementType);
----------------
Seems like there should be a llvm_unreachable after this error is emitted


================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:355
+  auto boxTy = fir::BoxType::get(elementType);
+  return exv.match(
+      [&](const fir::ArrayBoxValue &box) -> mlir::Value {
----------------
I don't quite find what the `.match(` call resolves to on the `fir::ExtendedValue` class? Any tip?


================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:357
+      [&](const fir::ArrayBoxValue &box) -> mlir::Value {
+        auto s = createShape(loc, exv);
+        return create<fir::EmboxOp>(loc, boxTy, itemAddr, s);
----------------
(same below)


================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:382
+      },
+      [&](const auto &) -> mlir::Value {
+        return create<fir::EmboxOp>(loc, boxTy, itemAddr);
----------------
Can you comment on what this is supposed to match?


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:355
+  mlir::Value load;
+  auto addr = st.memref();
+  auto stEleTy = fir::dyn_cast_ptrOrBoxEleTy(addr.getType());
----------------
not sure if it is a Value here?


================
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;
+          }
----------------
I commented on this before I believe: you have two map lookup when one would be enough I think:



================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:644
+                        ArrayOp update,
+                        const std::function<void(mlir::Value)> &assignElement,
+                        mlir::Type lhsEltRefType) const {
----------------



================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:671
+      assignElement(coor);
+      auto *storeOp = useMap.lookup(loadOp);
+      auto store = mlir::cast<ArrayMergeStoreOp>(storeOp);
----------------
(previous use of `useMap.lookup()` in this function spelled out the type)


================
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);
----------------
This branch may be worth a comment.

In general the entire pattern is a bit light on documentation.


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:730
+          llvm::report_fatal_error("not a legal reference type");
+        }
+      } else {
----------------
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>());
```


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:468
+            LLVM_DEBUG(llvm::dbgs() << " access: " << *acc << '\n');
+            if (mlir::isa<ArrayFetchOp, ArrayUpdateOp, ArrayModifyOp>(acc)) {
+              if (useMap.count(acc)) {
----------------
mehdi_amini wrote:
> Can this check fail? Right now the contract of `arrayAccesses()` is to only return these three operations, and `conflictOnMerge` would fail if there were any other op.
> Can this be replace with an assert here?
You marked it done, but I don't see it done in the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111337