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

[Pete Steinfeld via Phabricator via flang-commits]

PeteSteinfeld requested changes to this revision.
PeteSteinfeld added a comment.
This revision now requires changes to proceed.

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



================
Comment at: flang/include/flang/Optimizer/Builder/HLFIRTools.h:228
+/// Compute the lower and upper bounds given a fir.shape or fir.shape_shift
+/// (fir.shift are not allowed here).
+llvm::SmallVector<std::pair<mlir::Value, mlir::Value>>
----------------
Should read "is not allowed"


================
Comment at: flang/lib/Lower/ConvertExprToHLFIR.cpp:110
+    // Arrays with non default lower bounds or dynamic length or dynamic extent
+    // needs a fir.box to hold the dynamic or lower bound information.
+    if (fir::hasDynamicSize(resultValueType) ||
----------------
Should read "need a fir.box"


================
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>());
+  }
----------------
Shouldn't this function be checking the value of the lower bound?


================
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
----------------
This sentence is a little awkward.  Does this work -- "Called from contexts where the component is expected to be the base of an ArrayRef."?


================
Comment at: flang/lib/Lower/ConvertExprToHLFIR.cpp:449
+
+  // Returns the <BaseType, ComponentType> pair, compute partInfo.base,
+  // partInfo.componentShape and partInfo.typeParams, but does not set the
----------------
"compute" should read "computes".


================
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>
----------------
Did you leave something out here?  The sentence "This entry points allows." seems incomplete.


================
Comment at: flang/lib/Optimizer/Builder/HLFIRTools.cpp:348-349
+                 mlir::Value shape) {
+  assert(shape.getType().isa<fir::ShapeShiftType>() ||
+         shape.getType().isa<fir::ShapeType>() && "shape must contain extents");
+  auto extents = getExplicitExtents(shape);
----------------
When I build this code, I get a compilation error here.  I'm using GCC 9.3.0.  Here's the relevant excerpt from the build's log file:
```
/local/home/psteinfeld/main/cref/flang/lib/Optimizer/Builder/HLFIRTools.cpp: In function ‘llvm::SmallVector<std::pair<mlir::Value, mlir::Value> > hlfir::genBounds(mlir::Location, fir::FirOpBuilder&, mlir::Value)’:
/local/home/psteinfeld/main/cref/flang/lib/Optimizer/Builder/HLFIRTools.cpp:349:48: error: suggest parentheses around ‘&&’ within ‘||’ [-Werror=parentheses]
  349 |          shape.getType().isa<fir::ShapeType>() && "shape must contain extents");
      |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
At global scope:
cc1plus: error: unrecognized command line option ‘-Wno-ctad-maybe-unsupported’ [-Werror]
cc1plus: all warnings being treated as errors
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141470