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

[Valentin Clement via Phabricator via llvm-commits]

clementval marked an inline comment as done.
clementval added a comment.

The latest update to this patch add a new `flang/test/Fir/array-value-copy.fir` file that cover the `array-value-copy` in more details.



================
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)
----------------
mehdi_amini wrote:
> This condition does not seem tested
Test added in `flang/test/Fir/array-value-copy.fir` cover this part as well. 


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:583
+  if (skipOrig)
+    originated.assign(indices.begin(), indices.end());
+  else
----------------
mehdi_amini wrote:
> This case seems untested.
Test added in `flang/test/Fir/array-value-copy.fir` cover this code. 


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:597
+        loc, resTy, result,
+        llvm::ArrayRef<mlir::Value>{originated}.drop_front(dimension));
+  return result;
----------------
mehdi_amini wrote:
> I can delete the 4 lines above (the if and its body) and the test still passes
The test added in `flang/test/Fir/array-value-copy.fir` that uses array of derived types is covering this part of the code. 


================
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))
----------------
mehdi_amini wrote:
> This condition doesn't seem tested
This will require `fir.array_acccess`/`fir.array_amend` to be tested correctly with character arrays. Remove it from this upstreaming patch for now. 


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:618
+      return {};
+    if (auto co = mlir::dyn_cast<fir::ConvertOp>(op))
+      return recoverTypeParams(co.value());
----------------
mehdi_amini wrote:
> This condition doesn't seem tested.
This will require `fir.array_acccess`/`fir.array_amend` to be tested correctly with character arrays. Remove it from this upstreaming patch for now. 


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:635
+    llvm::report_fatal_error("unexpected buffer");
+  }
+
----------------
mehdi_amini wrote:
> Actually this entire method isn't covered by the test
This will require `fir.array_acccess`/`fir.array_amend` to be tested correctly with character arrays. Remove it from this upstreaming patch for now. 


================
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);
----------------
mehdi_amini wrote:
> The reverse can be deleted here and the test still pass.
The test with multidimensional array added in `flang/test/Fir/array-value-copy.fir` make sure the reverse indices is tested correctly. 


================
Comment at: flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp:833
+    if (fir::isa_ref_type(fetch.getType()))
+      rewriter.replaceOp(fetch, coor);
+    else
----------------
mehdi_amini wrote:
> This case seems untested
This cannot be tested currently with what is upstreamed. Remove it for the moment. 


================
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);
----------------
mehdi_amini wrote:
> Commenting out this line does not break the test
Test covering `fir.array_update` conversion have been added in `flang/test/Fir/array-value-copy.fir`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111337