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

[Jean Perier via Phabricator via flang-commits]

jeanPerier marked 2 inline comments as done.
jeanPerier added inline comments.


================
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,
----------------
PeteSteinfeld wrote:
> I didn't see anywhere that this interface was called.  Is that OK?
It is called twice in `RuntimeTempStrategy` constructor (`initialBoxValue = builder.createBox(...)` around line 319 and 338).


================
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,
----------------
PeteSteinfeld wrote:
> What does the "extent" argument represent?  Does this constructor only apply to singly dimensioned arrays?
Yes, array constructors are 1 ranked arrays. Extent is the pre-computed extent of the array constructor, if it was possible to compute it. I'll make it an std::optional so that it is more clear that it may be null here.
I added comment about the arguments.


================
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,
----------------
PeteSteinfeld wrote:
> This is the only place where the data member `declare` gets assigned a value in this constructor.  Is that OK?
Yes, I added a comment in the else case:

```
The declare operation cannot be emitted in this case since the final
array constructor has not yet been allocated. Instead, the resulting
temporary variable will be extracted from the allocatable descriptor
after all the API calls.
```

And made the member an std::optional so that it is clearer that this member may not be always set (mlir::Value and operation always have a sort of "optional" state were they can be left null and `if (value)` will tell if it was not yet set, but using std::optional better convey the intent).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144513