[flang-commits] [PATCH] D144102: [flang][hlfir] Array constructor lowering [part 1/4]

[Jean Perier via Phabricator via flang-commits]

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


================
Comment at: flang/lib/Lower/ConvertArrayConstructor.cpp:48
+//   the array constructor, potentially reshaped, to an array variable.
+//
+//   The array constructor lowering looks like:
----------------
PeteSteinfeld wrote:
> What about the case where there's an ac-implied-do-control, and the expression invokes an intrinsic that yields a constant?  Also, are you handling array constructors for arrays of derived types?
> What about the case where there's an ac-implied-do-control, and the expression invokes an intrinsic that yields a constant?

By "the expression", do you mean and ac-implied-do control bound (`[i, i=1, abs(-42)]`), or the expression inside the ac-implied-do (`[(acos(42.), i=1,n)]`)?
In both cases, the front-end would anyway fold the result and we would not see it, but even if we did we would use the third strategy: lower it as an hlfir.elemental.

 > Also, are you handling array constructors for arrays of derived types?

The current code is blocking derived type so that I can add tests for this case in its own patch (see TODO in `LengthAndTypeCollector<Fortran::evaluate::SomeDerived>` line 277). Derived type will use the runtime strategy I think, mainly to ensure we do not call user defined assignment for to copy them inside the temporary for now (other than that, there is no deep reason the other two strategies would not work for them too).


================
Comment at: flang/lib/Lower/ConvertArrayConstructor.cpp:87
+    llvm::SmallVector<mlir::Value, 1> extents{extent};
+    mlir::Value tempStorage = builder.createHeapTemporary(
+        loc, declaredType, tempName, extents, lengths);
----------------
tblah wrote:
> I thought the plan was to centralize all array temporary generation in the hlfir bufferization pass? I guess there is not a way to write "give me an uninitialised hlfir.expr which will be given a buffer later"?
> 
> It would be nice if expression buffer allocation could be done in one place so that, eventually, the stack arrays pass (and all its messy data flow analysis) can go away.
> 
> Another option would be to always allocate temporaries on the stack (no matter if `-fstack-arrays` or `-Ofast` were specified) and rely on the memory-allocation-opt pass to move large allocations to the heap, if that's desired. As I understand it, non-temporary array variables will already always be allocated on the stack (no matter their size).
> 
> Feel free to ignore this comment if this was a deliberate design decision.
> I thought the plan was to centralize all array temporary generation in the hlfir bufferization pass?

It is as much as possible. But in some case, it is hard to implement the Fortran concept without using memory.
The plan is to "pay the abstraction price" of removing the early temp creation where it makes an impact, but lowering may still resort to creating temps in some cases.

> I guess there is not a way to write "give me an uninitialised hlfir.expr which will be given a buffer later"?

Indeed, I think doing so and using chain of inserts would have a quite complex code generation to ensure a new temporary is not created at each insert.

> It would be nice if expression buffer allocation could be done in one place so that, eventually, the stack arrays pass (and all its messy data flow analysis) can go away.

Agree. I will look into it. I think I first need to propagate the option here, so I will do it in a separate patch.

To be fair, I am not saying it is impossible/undesired to have a powerful abstraction to represent all kinds of array constructors in HLFIR. I think it will need some careful design to cope with all cases and ensure it "plugs" well with the optimization passes. I am thinking we could have something like:

```
%expr = hlfir.array_ctor [%optional_shape] [%optional_length_parameters] {
  fir.do_loop {
    hlfir.push_value %acValue ...
  }
  hlfir.push_value %acValue ....
}
```

The obvious benefit is that if %expr is then assigned, we could assume that the left hand side (if it is not a whole allocatable assignment) has the right storage size, and use that info to create the temp if needed, or if aliasing analysis proves the LHS is not used inside the array_ctor region.
Same if we have Array + array_ctor_hard_to_predict_size, we could use conformability requirement to deduce the array constructor size.

I am not going that way now because I would like to better think the implication of such a new operation with region.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144102