[PATCH] D115333: [flang][codegen] Add a conversion for `fir.coordinate_of` - part 2

[Diana Picus via Phabricator via llvm-commits]

rovka accepted this revision.
rovka added a comment.
This revision is now accepted and ready to land.

In D115333#3195069 <https://reviews.llvm.org/D115333#3195069>, @awarzynski wrote:

> - Replace a couple of `TODO`s with an error
> - Make sure that `!fir.ptr` is supported too (alongside `!fir.ref`)

Is there a test for fir.ptr?

Other than that, LGTM (with microscopic nits).



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2673
+      return (!subEle) && (sz == 1);
+    return subEle && (i >= sz);
+  }
----------------
awarzynski wrote:
> rovka wrote:
> > The i >= sz part is strange. Unless I'm missing something, that can only happen in the case of a sequential type, but the [[ https://github.com/llvm/llvm-project/blob/1aa59ff2f789776ebfa2d4b315fd3ea589652b4a/flang/include/flang/Optimizer/Dialect/FIROps.td#L1636 | docs ]] say "When computing the coordinate of an array element, the rank of the array must be known and the number of indexing expressions must equal the rank of the array". IIUC that means i has to be exactly equal to sz, no?
> > The i >= sz part is strange. Unless I'm missing something, that can only happen in the case of a sequential type, but the docs say "When computing the coordinate of an array element, the rank of the array must be known and the number of indexing expressions must equal the rank of the array". IIUC that means i has to be exactly equal to sz, no?
> 
> Thanks for the link! Yeah, I agree that this is confusing. See this [[ https://github.com/flang-compiler/f18-llvm-project/blob/fir-dev/flang/test/Fir/coordinateof.fir#L45-L55 | example ]]. It's inconsistent with the documentation that you referred to in your comment. Perhaps the docs should be updated to read "(...) must not be greater than the rank of the array." instead of "(...) must equal the rank of the array.". WDTY? 
Ok, I was not aware of that example :) Let's update the docs then.
Also, thanks for the sz->numOfCoors, that's more readable.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2679
+  /// constant shape along the path.
+  bool arraysHaveKnownShape(mlir::Type type, mlir::ValueRange coors) const {
+    const auto sz = coors.size();
----------------
awarzynski wrote:
> rovka wrote:
> > Same as above, shouldn't this be in the verify function? (There seems to be something along these lines, but it doesn't look very complete)
> Hm, but both `true` and `false` might lead to a valid case. Unless I missed something.
Yes, sorry, big patch, I think I had paged out some parts of it by the time I was done reviewing :D


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2949
+    if (baseObjectTy.dyn_cast<fir::ReferenceType>() ||
+        baseObjectTy.dyn_cast<fir::PointerType>())
+      return doRewriteRef(coor, ty, operands, loc, rewriter);
----------------
Nit: I'm not 100% sure, but can you use baseObjectTy.isa<fir::ReferenceType, fir::PointerType>()?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2978
+    return type.isa<fir::SequenceType>() || type.isa<fir::RecordType>() ||
+           type.isa<mlir::TupleType>();
+  }
----------------
Same as above.


================
Comment at: flang/test/Fir/convert-to-llvm.fir:2474
+
+// 5.4 `fir.tuple`
+func @test_coordinate_of_tuple(%tup : !fir.ref<tuple<!fir.ref<i32>>>) {
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115333