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

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

Love the tests! I think the code allows more cases though, e.g. array of array, array inside derived etc. I think we should have lots of mixed testcases like that, since the code is particularly hard to follow :-/  Or if we don't support that we should consider erroring out early (sorry if we already do and I just missed it).



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2650
+
+  bool validCoordinate(mlir::Type type, mlir::ValueRange coors) const {
+    const auto sz = coors.size();
----------------
Why is this here and not in the [[ https://github.com/llvm/llvm-project/blob/1aa59ff2f789776ebfa2d4b315fd3ea589652b4a/flang/lib/Optimizer/Dialect/FIROps.cpp#L882 | verifier ]]? Or is this just the subset of coordinate stuff that's supported at the moment in the CodeGen? If so, then maybe we should call it supportedCoordinate instead and add a comment that we intend to support more in the future (and that eventually the whole function should disappear once everything is supported).


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2673
+      return (!subEle) && (sz == 1);
+    return subEle && (i >= sz);
+  }
----------------
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?


================
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();
----------------
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)


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2800
+    assert(currentObjTy.dyn_cast<fir::SequenceType>() ||
+           currentObjTy.dyn_cast<fir::RecordType>() && "Unsupported type");
+    bool hasSubdimension = hasSubDimensions(currentObjTy);
----------------
Nit: Should we also support mlir::TupleType, like we do in hasSubDimensions?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2805
+    if (!hasSubdimension)
+      columnIsDeferred = true;
+
----------------
Nit: You could just have bool columnIsDeferred = !hasSubdimension;


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2808
+    if (!validCoordinate(currentObjTy, operands.drop_front(1)))
+      TODO(loc, "coordinate has incorrect dimension");
+
----------------
If it's incorrect, then it should be an error rather than a TODO. If it's just unsupported yet, then I think the message should say that instead :) 


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2810
+
+    // if arrays has known shape
+    const bool hasKnownShape =
----------------
Nit: This comment is superfluous (and ungrammatical), I think you can skip it. hasKnownShape is clear enough imo.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2854
+        if (!currentObjTy)
+          TODO(loc, "invalid coordinate/check failed");
+
----------------
Is this really a TODO? Sounds more like an error...


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2900
+    }
+    TODO(loc, "fir.coordinate_of base operand has unsupported type");
   }
----------------
Error?


================
Comment at: flang/test/Fir/convert-to-llvm.fir:2218
+func @coordinate_array_known_size_2d(%arg0: !fir.ref<!fir.array<10 x 10 x i32>>, %arg1 : index) {
+   %q = fir.coordinate_of %arg0, %arg1, %arg1 : (!fir.ref<!fir.array<10 x 10 x i32>>, index, index) -> !fir.ref<i32>
+   return
----------------
Nit: I think it would be worthwhile to introduce a %arg2 here


================
Comment at: flang/test/Fir/convert-to-llvm.fir:2230
+// 5.2. `fir.derived`
+func @coordinate_ref_derived(%arg0: !fir.ref<!fir.type<dervied_4{field_1:i32, field_2:i32}>>) {
+  %idx = fir.field_index field_2, !fir.type<dervied_4{field_1:i32, field_2:i32}>
----------------
Nit: I think you can skip the definition for derived_4 on the other 2 lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115333