[flang-commits] [PATCH] D141470: [flang] Lower component-ref to hlfir.designate

[Jean Perier via Phabricator via flang-commits]

jeanPerier added a comment.

In D141470#4043709 <https://reviews.llvm.org/D141470#4043709>, @PeteSteinfeld wrote:

> A few nits and questions, but my build failed and should be fixed.

Thanks @PeteSteinfeld, I rephrased the comments and the warning should be gone.



================
Comment at: flang/lib/Lower/ConvertExprToHLFIR.cpp:385-389
+  static bool hasNonDefaultLowerBounds(const PartInfo &partInfo) {
+    return partInfo.resultShape &&
+           (partInfo.resultShape.getType().isa<fir::ShiftType>() ||
+            partInfo.resultShape.getType().isa<fir::ShapeShiftType>());
+  }
----------------
PeteSteinfeld wrote:
> Shouldn't this function be checking the value of the lower bound?
You mean, to check that they are not all ones ?

No, this was already done when generating the fir.shift/fir.shape_shift (either here via the `hasNonDefaultLowerBounds` that checks for the value for component array lower bounds (when possible of course), or when converting symbols specification expression at [1] before lowering the subprogram body). Worst case, it is not a correctness issue to do computation with lower bounds that are all ones.

[1] https://github.com/llvm/llvm-project/blob/b71bbbb64ff92184e13a793b71982df4cdee0271/flang/lib/Lower/ConvertVariable.cpp#L1192


================
Comment at: flang/lib/Lower/ConvertExprToHLFIR.cpp:426-427
                    PartInfo &partInfo) {
-    TODO(getLoc(), "lowering component to HLFIR");
+    // Called From everywhere expects if the component is the base
+    // of an ArrayRef.
+    // In these other cases, the component cannot be an array if the
----------------
PeteSteinfeld wrote:
> This sentence is a little awkward.  Does this work -- "Called from contexts where the component is expected to be the base of an ArrayRef."?
Thanks, I meant the reverse. I rephrased your suggestion to  "Called from contexts where the component is not the base of an ArrayRef."


================
Comment at: flang/lib/Lower/ConvertExprToHLFIR.cpp:451
+  // partInfo.componentShape and partInfo.typeParams, but does not set the
+  // partInfo.resultShape yet. This entry points allows.
+  std::pair<mlir::Type, mlir::Type>
----------------
PeteSteinfeld wrote:
> Did you leave something out here?  The sentence "This entry points allows." seems incomplete.
Whoops. Rephrased to:  "The result shape will be computed after processing a following ArrayRef, if any, and in "visit" otherwise."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141470