[flang-commits] [PATCH] D144513: [flang][hlfir] Array constructor lowering [part 3/4]

[Pete Steinfeld via Phabricator via flang-commits]

PeteSteinfeld added a comment.

All builds and tests correctly.  I had some nits and questions.  But I'm not sure I understand this well enough to approve.  Valentin should take a look.



================
Comment at: flang/include/flang/Optimizer/Builder/FIRBuilder.h:341
 
+  mlir::Value createBox(mlir::Location loc, mlir::Type boxType,
+                        mlir::Value addr, mlir::Value shape, mlir::Value slice,
----------------
I didn't see anywhere that this interface was called.  Is that OK?


================
Comment at: flang/lib/Lower/ConvertArrayConstructor.cpp:300
+  /// The temporary is only created if the extents and length parameters are
+  /// already know. Otherwise, the handling of the allocation (and reallocation
+  /// is left up to the runtime).
----------------
"know" should be "known".


================
Comment at: flang/lib/Lower/ConvertArrayConstructor.cpp:301
+  /// already know. Otherwise, the handling of the allocation (and reallocation
+  /// is left up to the runtime).
+  RuntimeTempStrategy(mlir::Location loc, fir::FirOpBuilder &builder,
----------------
This would make more sense if the ")" appeared after the word "reallocation".


================
Comment at: flang/lib/Lower/ConvertArrayConstructor.cpp:303
+  RuntimeTempStrategy(mlir::Location loc, fir::FirOpBuilder &builder,
+                      fir::SequenceType declaredType, mlir::Value extent,
+                      llvm::ArrayRef<mlir::Value> lengths,
----------------
What does the "extent" argument represent?  Does this constructor only apply to singly dimensioned arrays?


================
Comment at: flang/lib/Lower/ConvertArrayConstructor.cpp:316
+      mlir::Value shape = builder.genShape(loc, extents);
+      declare = builder.create<hlfir::DeclareOp>(
+          loc, tempStorage, tempName, shape, lengths,
----------------
This is the only place where the data member `declare` gets assigned a value in this constructor.  Is that OK?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144513