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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

I took a few minutes to evaluate more precisely the level of testing, see inline: we're very far from covering our bases here (and this is far from exhaustive!).

I would suggest going back to first principles in terms of testing and building some solid and minimal tests for the passes you're trying to upstream.
I know you're not necessarily the author of the passes, and I suggest to involve the author(s) of the code ahead of time to improve the testing before attempting to upstream all of this.



================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:552
+  assert(result.empty());
+  if (auto boxTy = loadOp.memref().getType().dyn_cast<fir::BoxType>()) {
+    auto rank = fir::dyn_cast_ptrOrBoxEleTy(boxTy)
----------------
This condition does not seem tested


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:583
+  if (skipOrig)
+    originated.assign(indices.begin(), indices.end());
+  else
----------------
This case seems untested.


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:597
+        loc, resTy, result,
+        llvm::ArrayRef<mlir::Value>{originated}.drop_front(dimension));
+  return result;
----------------
I can delete the 4 lines above (the if and its body) and the test still passes


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:617
+    if (!fir::hasDynamicSize(fir::dyn_cast_ptrEleTy(val.getType())))
+      return {};
+    if (auto co = mlir::dyn_cast<fir::ConvertOp>(op))
----------------
This condition doesn't seem tested


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:618
+      return {};
+    if (auto co = mlir::dyn_cast<fir::ConvertOp>(op))
+      return recoverTypeParams(co.value());
----------------
This condition doesn't seem tested.


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:635
+    llvm::report_fatal_error("unexpected buffer");
+  }
+
----------------
Actually this entire method isn't covered by the test


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:661
+    // Reverse the indices so they are in column-major order.
+    std::reverse(indices.begin(), indices.end());
+    auto ty = getEleTy(arrTy);
----------------
The reverse can be deleted here and the test still pass.


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:833
+    if (fir::isa_ref_type(fetch.getType()))
+      rewriter.replaceOp(fetch, coor);
+    else
----------------
This case seems untested


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:860
+    patterns1.insert<ArrayFetchConversion>(context, useMap);
+    patterns1.insert<ArrayUpdateConversion>(context, analysis, useMap);
+    patterns1.insert<ArrayModifyConversion>(context, analysis, useMap);
----------------
Commenting out this line does not break the test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111337