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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski marked 6 inline comments as done.
awarzynski added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2650
+
+  bool validCoordinate(mlir::Type type, mlir::ValueRange coors) const {
+    const auto sz = coors.size();
----------------
rovka wrote:
> 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).
> Why is this here and not in the verifier? Or is this just the subset of coordinate stuff that's supported at the moment in the CodeGen? 

If we move this to the verifier then we will effectively change the semantics of `!fir.coordinate_of`. That's something that should probably be discussed first. I did experiment with this idea and saw quite a few new regressions, so clearly other parts of the compiler do generate code that this method deems as unsupported.

IIUC, the purpose of this method is to capture cases not supported by this conversion rather then to refine the semantics of `!fir.coordinate_of`. I will rename it and add some comments to clarify this. I do feel that some cases captured here are genuinely invalid and should indeed be rejected by the verifier, but we'd probably have to do on a case-by-case basis. How about a `TODO` for now?


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


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


================
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);
----------------
rovka wrote:
> Nit: Should we also support mlir::TupleType, like we do in hasSubDimensions?
We should. In fact, this `assert` is not needed here. [[ https://github.com/llvm/llvm-project/blob/1aa59ff2f789776ebfa2d4b315fd3ea589652b4a/flang/lib/Optimizer/Dialect/FIROps.cpp#L892-L894 | FIROps.cpp ]] takes care of this. I will remove this.


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


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2808
+    if (!validCoordinate(currentObjTy, operands.drop_front(1)))
+      TODO(loc, "coordinate has incorrect dimension");
+
----------------
rovka wrote:
> 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 :) 
Let me update the message then :)


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115333