This is an archive of the discontinued LLVM Phabricator instance.

[flang][hlfir] Array constructor lowering [part 1/4]
ClosedPublic

Authored by jeanPerier on Feb 15 2023, 6:23 AM.

Details

Summary

This is the first and biggest chunk that introduces support for
array constructor to HLFIR.

This patch:

  • adds a new ConvertArrayConstructor.cpp that centralizes the code dealing with array constructor lowering.
  • introduces a framework to lower array constructor according to different strategies: A common analysis of the array constructor is done, and based on that, a lowering startegy is selected and driven through the ac-values of the array constructor. See ConvertArrayConstructor.cpp comments for more details.
  • implements the first strategy that creates a temporary inlined and updates it with inlined code. This strategy can only be used if the temporary can be pre-allocated (i.e: the extents and length parameters can be pre-computed without evaluating any ac-values), and if all the ac-value expressions are scalars.

For the sake of simplicity, characters and derived type will be enabled
once all the strategies are added.

Diff Detail

Event Timeline

jeanPerier created this revision.Feb 15 2023, 6:23 AM
jeanPerier requested review of this revision.Feb 15 2023, 6:23 AM
tblah added a subscriber: tblah.Feb 15 2023, 6:49 AM

Add empty namespaces around implementation classes and structs as per
LLVM developer guidelines.

tblah added inline comments.Feb 15 2023, 9:38 AM
flang/lib/Lower/ConvertArrayConstructor.cpp
88

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.

LGTM

flang/lib/Lower/ConvertArrayConstructor.cpp
38
107
115
393–394
PeteSteinfeld accepted this revision.Feb 15 2023, 2:36 PM

Aside from some nits and questions, all builds and tests correctly and looks good.

I didn't understand Valentin's review. He says "LGTM", but it looks like he didn't approve. You might want to double check with him.

flang/lib/Lower/ConvertArrayConstructor.cpp
49

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?

70–71

This should either be "lower an array constructor" or "lower array constructors"

148

"become" should be "becomes"

152

"reduce" should be "reduces"

298

"require" should be "requires"

311

"are used" should be "is used"

430

"try gather" should be "try to gather"

This revision is now accepted and ready to land.Feb 15 2023, 2:36 PM
clementval accepted this revision.Feb 16 2023, 12:07 AM

I didn't understand Valentin's review. He says "LGTM", but it looks like he didn't approve. You might want to double check with him.

Just forgot to select the option.

jeanPerier marked 10 inline comments as done.Feb 16 2023, 2:45 AM
jeanPerier added inline comments.
flang/lib/Lower/ConvertArrayConstructor.cpp
49

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).

88

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.

jeanPerier marked an inline comment as done.

Fix typos in comments. Thanks for the review.

tblah added inline comments.Feb 16 2023, 2:52 AM
flang/lib/Lower/ConvertArrayConstructor.cpp
88

Thanks for explaining. I think it is fine to come back to this at a later time

This revision was automatically updated to reflect the committed changes.