[PATCH] D115077: [fir] Add array operations documentation
Mehdi AMINI via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 9 16:03:26 PST 2021
mehdi_amini added inline comments.
================
Comment at: flang/docs/FIRArrayOperations.md:94
+ fir.array_merge_store %v, %r to %a : !fir.ref<!fir.array<?x?xf32>>
+```
+
----------------
clementval wrote:
> mehdi_amini wrote:
> > clementval wrote:
> > > mehdi_amini wrote:
> > > > clementval wrote:
> > > > > mehdi_amini wrote:
> > > > > > It's not clear to me in this example why array_merge_store needs %v?
> > > > > > Why isn't it just `fir.array_store %r to %a` enough? Seems like the array_update should have all the information here.
> > > > > In this example it has everything but it's not always the case like in the example below:
> > > > >
> > > > > ```
> > > > > func @conversion_with_temporary(%arr0 : !fir.ref<!fir.array<10xi32>>) {
> > > > > %c10 = arith.constant 10 : index
> > > > > %1 = fir.shape %c10 : (index) -> !fir.shape<1>
> > > > > %2 = fir.array_load %arr0(%1) : (!fir.ref<!fir.array<10xi32>>, !fir.shape<1>) -> !fir.array<10xi32>
> > > > > %c10_i64 = arith.constant 10 : i64
> > > > > %3 = fir.convert %c10_i64 : (i64) -> index
> > > > > %c1_i64 = arith.constant 1 : i64
> > > > > %c-1_i64 = arith.constant -1 : i64
> > > > > %4 = fir.shape %c10 : (index) -> !fir.shape<1>
> > > > > %5 = fir.slice %c10_i64, %c1_i64, %c-1_i64 : (i64, i64, i64) -> !fir.slice<1>
> > > > > %6 = fir.array_load %arr0(%4) [%5] : (!fir.ref<!fir.array<10xi32>>, !fir.shape<1>, !fir.slice<1>) -> !fir.array<10xi32>
> > > > > %c1 = arith.constant 1 : index
> > > > > %c0 = arith.constant 0 : index
> > > > > %7 = arith.subi %3, %c1 : index
> > > > > %8 = fir.do_loop %arg0 = %c0 to %7 step %c1 unordered iter_args(%arg1 = %2) -> (!fir.array<10xi32>) {
> > > > > %9 = fir.array_fetch %6, %arg0 : (!fir.array<10xi32>, index) -> i32
> > > > > %10 = fir.array_update %arg1, %9, %arg0 : (!fir.array<10xi32>, i32, index) -> !fir.array<10xi32>
> > > > > fir.result %10 : !fir.array<10xi32>
> > > > > }
> > > > > fir.array_merge_store %2, %8 to %arr0 : !fir.array<10xi32>, !fir.array<10xi32>, !fir.ref<!fir.array<10xi32>>
> > > > > return
> > > > > }
> > > > > ```
> > > > I don't quite get the different in this example?
> > > > If I track the loop, you have the loaded array used a initial induction value, then you update one element, and then pass it along to the next iteration until the end. %8 seems to contain the array fully updated?
> > > Agreed you can track it back all the way to `fir.array_load`. Since we know it during lowering why make it harder to track it and not just having it right away?
> > My comment explaining how %8 should be enough isn't about the ability tracking it back to the fir.array_load, this is about the SSA value being self consistent in the semantics model.
> >
> > It isn't clear to me in the description of fir.array_merge_store what is the first operand useful for.
> >
> > I suspect it helps tying thing together in some lowering later potentially, but I'm not sure how (and it isn't documented).
> Array expressions work with a context (such as a statement). FIR does not model Fortran statements directly. Instead we "bound" a region of FIR with `array_load`s and an `array_merge_store`.
> The analysis looks for these Ops as markers that define the region of IR to be considered for analysis.
I understand the "statement problem" in Fortran, but I don't quite make the direct connection in FIR. Basically you're saying "we're propagating the 'bound' of the statement here", but that does not really say why it is needed or useful?
================
Comment at: flang/docs/FIRArrayOperations.md:122
+
+It is only possible to use `array_fetch` on an `array_load` result value.
+
----------------
clementval wrote:
> mehdi_amini wrote:
> > clementval wrote:
> > > mehdi_amini wrote:
> > > > clementval wrote:
> > > > > mehdi_amini wrote:
> > > > > > Why this restriction?
> > > > > The array "abstraction" in FIR is not meant to be used anywhere. Fortran itself doesn't have "array values" in any sane sense of that term. To avoid to introduce arrays universally in FIR we restrict the use of the abstraction to precisely where we want it and where it is useful.
> > > > >
> > > > I don't feel this is a clear answer. What does "To avoid to introduce arrays universally" means for example?
> > > > Restricting the use of array makes sense: that means that `fir.add` will not accept to add two arrays for example, but that does not explain clearly to me the link between fetch and load.
> > > >
> > > >
> > > > In general: an SSA value is equal to another SSA value, what you're saying here is that inside the IR this isn't the case and a transformation that would make an SSA value a block argument for example isn't valid.
> > > >
> > > > This seems fragile and extremely uncommon (I don't know of any precent in any dialect I worked on), so I'd insist to look for any alternative to such a strong design constraint: the bar to justify this should be really high and we're not there yet IMO.
> > > I think the restriction is probably not phrased correctly. What we want to mean is that the value must trace back transitively to an `array_load` as the dominating source.
> > > We have places where those SSA value are block arguments.
> > >
> > > For example, this is totally fine FIR:
> > >
> > > ```
> > > %1 = array_load
> > > fir.loop ... iter_args (%2 = %1) ...
> > > %3 = array_fetch %2
> > > ```
> > >
> > ```
> > For example, this is totally fine FIR:
> >
> > %1 = array_load
> > fir.loop ... iter_args (%2 = %1) ...
> > %3 = array_fetch %2
> > ```
> >
> > Seems like %2 after the first iteration will be the result of the yield of the previous iteration: what will produce this value?
> > Unless it is an array_load, your condition does not seem met to me.
> This is a more complete example:
>
> ```
> %7 = fir.array_load %arg0 [%6] : (!fir.box<!fir.array<?xf32>>, !fir.slice<1>)
> -> !fir.array<?xf32>
> ...
> %18 = fir.do_loop %arg5 = %c0 to %17 step %c1 iter_args(%arg6 = %7) -> (!fir.array<?xf32>) {
> %19 = fir.array_fetch %arg6, %arg5 : (!fir.array<?xf32>, index) -> f32
> %20 = fir.array_update %arg6, %19, %arg5 : (!fir.array<?xf32>, f32, index) -> !fir.array<?xf32>
> fir.result %20 : !fir.array<?xf32>
> }
> fir.array_merge_store %7, %18 to %arg0[%6] : !fir.array<?xf32>, !fir.array<?xf32>, !fir.box<!fir.array<?xf32>>, !fir.slice<1>
> ```
Right, so here `array_fetch` will consume the result of the `array_update` after the first iteration and not the result from an `array_load`.
It may be a problem of definition, when you write "that can be trace back transitively to an `array_load` " ; do you see `array_update` as "transparent" in here? If so what is the list of such "transparent" operations?
================
Comment at: flang/docs/FIRArrayOperations.md:195
+ %new_v = fir.array_amend %v, %2 : (!fir.array<?x?xT>, !fir.ref<T>) -> !fir.array<?x?xT>
+```
+
----------------
clementval wrote:
> mehdi_amini wrote:
> > clementval wrote:
> > > mehdi_amini wrote:
> > > > I wonder why array_amend is needed at all actually.
> > > > When can't it just be replaced with a load of the `ref<T>` and an `array_update`?
> > > In some cases we do not want to load derived types/characters because they may have non-compile time constant sizes (cannot be loaded/might be big aggregates for which load/store is problematic (cause long LLVM compile time + asserts at a certain point).
> > Can you document this with an example illustrating such a case?
> Here is an example with character assignment.
>
> Fortran
> ```
> subroutine foo(c1, c2, n)
> integer(8) :: n
> character(n) :: c1(:), c2(:)
> c1 = c2
> end subroutine
> ```
>
> That would results in a cleaned-up FIR that looks like this:
>
> ```
> func @_QPfoo(%arg0: !fir.box<!fir.array<?x!fir.char<1,?>>>, %arg1: !fir.box<!fir.array<?x!fir.char<1,?>>>, %arg2: !fir.ref<i64>) {
> %0 = fir.load %arg2 : !fir.ref<i64>
> %c0 = arith.constant 0 : index
> %1:3 = fir.box_dims %arg0, %c0 : (!fir.box<!fir.array<?x!fir.char<1,?>>>, index) -> (index, index, index)
> %2 = fir.array_load %arg0 : (!fir.box<!fir.array<?x!fir.char<1,?>>>) -> !fir.array<?x!fir.char<1,?>>
> %3 = fir.array_load %arg1 : (!fir.box<!fir.array<?x!fir.char<1,?>>>) -> !fir.array<?x!fir.char<1,?>>
> %c1 = arith.constant 1 : index
> %4 = arith.subi %1#1, %c1 : index
> %5 = fir.do_loop %arg3 = %c0 to %4 step %c1 unordered iter_args(%arg4 = %2) -> (!fir.array<?x!fir.char<1,?>>) {
> %6 = fir.array_access %3, %arg3 : (!fir.array<?x!fir.char<1,?>>, index) -> !fir.ref<!fir.char<1,?>>
> %7 = fir.array_access %arg4, %arg3 : (!fir.array<?x!fir.char<1,?>>, index) -> !fir.ref<!fir.char<1,?>>
> %false = arith.constant false
> %8 = fir.convert %7 : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<i8>
> %9 = fir.convert %6 : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<i8>
> fir.call @llvm.memmove.p0i8.p0i8.i64(%8, %9, %0, %false) : (!fir.ref<i8>, !fir.ref<i8>, i64, i1) -> ()
> %10 = fir.array_amend %arg4, %7 : (!fir.array<?x!fir.char<1,?>>, !fir.ref<!fir.char<1,?>>) -> !fir.array<?x!fir.char<1,?>>
> fir.result %10 : !fir.array<?x!fir.char<1,?>>
> }
> fir.array_merge_store %2, %5 to %arg0 : !fir.array<?x!fir.char<1,?>>, !fir.array<?x!fir.char<1,?>>, !fir.box<!fir.array<?x!fir.char<1,?>>>
> return
> }
> func private @llvm.memmove.p0i8.p0i8.i64(!fir.ref<i8>, !fir.ref<i8>, i64, i1)
> }
> ```
>
> We do not want to start loading `!fir.ref<!fir.char<1,?>>` here because it has dynamic length, and even if constant, could be way too long.
Uh, I'm not totally understanding all the pieces here, in particular you're converting the references to `!fir.char<1,?>` to simple i8 reference and use the N arguments for the memmove size, but at the same time you're saying that they are dynamic length.
> if constant, could be way too long.
You're saying that basically `array_amend` is a fused form of `load`+`update` because the optimizer bails out and can't reconstruct the fused form later in the lowering?
All of this may be fine: it just deserves a lot of documentation I think.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115077/new/
https://reviews.llvm.org/D115077
More information about the llvm-commits
mailing list