[PATCH] D115077: [fir] Add array operations documentation

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


================
Comment at: flang/docs/FIRArrayOperations.md:94
+  fir.array_merge_store %v, %r to %a : !fir.ref<!fir.array<?x?xf32>>
+```
+
----------------
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?


================
Comment at: flang/docs/FIRArrayOperations.md:122
+
+It is only possible to use `array_fetch` on an `array_load` result value.
+
----------------
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
```



================
Comment at: flang/docs/FIRArrayOperations.md:158
+The `array_access` operationis used to fetch the memory reference of an element
+in an array value.
+
----------------
mehdi_amini wrote:
> I'm not comfortable with this description: from our previous discussion as it is described it looks like this creates a mutable reference to an element in the immutable array.
> We converged last time on a semantics where this implicitly creates a new memory location with automatic-allocation scope and copy the value there, as well as remembering the original coordinates this value came from.
> But this is a bit iffy as well: it seems like a `Ref<i32>` can't be universally defined where it comes from an `array_access` or from a `fir.alloca`.
> 
> This seems a bit loosely defined to me right now.
Description updated. 


================
Comment at: flang/docs/FIRArrayOperations.md:184
+
+The `array_amend` operation marks an array value as having been changed via its
+reference. The reference into the array value is obtained via a
----------------
mehdi_amini wrote:
> You need to update the description of the "array type" to mention what this means.
Which "array type" are you referencing to? (`!fir.array`)?


================
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>
+```
+
----------------
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).


================
Comment at: flang/docs/FIRArrayOperations.md:274
+  return
+}
+```
----------------
clementval wrote:
> mehdi_amini wrote:
> > It'd help if this code was simplified, for example the alloca for the loop induction variable seems spurious and just adds noise, similarly for the redundant conversions.
> > The SSA value could get name for readability of the example as well.
> > And finally some inline comment in the code to describe it and map it to the original source code would really help I think.
> > 
> > ```
> > func @_QPs(%arg0: !fir.box<!fir.array<?x!fir.type<_QFsTt{m:i32}>>>, %arg1: !fir.ref<i32>, %arg2: !fir.ref<i32>) {
> >   %l = fir.load %arg1 : !fir.ref<i32>
> >   %l_index = fir.convert %1 : (i32) -> index
> >   %u = fir.load %arg2 : !fir.ref<i32>
> >   %u_index = fir.convert %u : (i32) -> index
> >   %c1 = arith.constant 1 : index
> >   // This is the "copy-in" array used on the RHS of the expression. It will be indexed into and loaded at each iteration.
> >   %array_a_src = fir.array_load %arg0 : (!fir.box<!fir.array<?x!fir.type<_QFsTt{m:i32}>>>) -> !fir.array<?x!fir.type<_QFsTt{m:i32}>>
> > 
> >   // This is the "seed" for the "copy-out" array on the LHS. It'll flow from iteration to iteration and gets
> >   // updated at each iteration.
> >   %array_a_dest_init = fir.array_load %arg0 : (!fir.box<!fir.array<?x!fir.type<_QFsTt{m:i32}>>>) -> !fir.array<?x!fir.type<_QFsTt{m:i32}>>
> >   
> >   %array_a_final = fir.do_loop %i = %l_index to %u_index step %c1 unordered iter_args(%array_a_dest = % array_a_dest_init) -> (!fir.array<?x!fir.type<_QFsTt{m:i32}>>) {
> >     // Compute indexing for the RHS and array the element.
> >     %u_minus_i = arith.subi %u, %I : index // u-i
> >     %u_minus_i_plus_one = arith.addi %u_minus_i, %c1: index // u-i+1
> >     %a_src_ref = fir.array_access %array_a_src, %u_minus_i_plus_one {Fortran.offsets} : (!fir.array<?x!fir.type<_QFsTt{m:i32}>>, index) -> !fir.ref<!fir.type<_QFsTt{m:i32}>>
> >     %a_src_elt = fir.load %a_src_ref : !fir.ref<!fir.type<_QFsTt{m:i32}>>
> > 
> >     // Get the reference to the element in the array on the LHS
> >     %a_dst_ref = fir.array_access %array_a_dest, %i {Fortran.offsets} : (!fir.array<?x!fir.type<_QFsTt{m:i32}>>, index) -> !fir.ref<!fir.type<_QFsTt{m:i32}>>
> > 
> >     // Store the value, and update the array
> >     fir.store %a_src_elt to %a_dst_ref : !fir.ref<!fir.type<_QFsTt{m:i32}>>
> >     %updated_array_a = fir.array_amend %array_a, %a_dst_ref : (!fir.array<?x!fir.type<_QFsTt{m:i32}>>, !fir.ref<!fir.type<_QFsTt{m:i32}>>) -> !fir.array<?x!fir.type<_QFsTt{m:i32}>>
> > 
> >     // Forward the current updated array to the next iteration.
> >     fir.result %updated_array_a : !fir.array<?x!fir.type<_QFsTt{m:i32}>>
> >   }
> >   // Store back the result by merging the initial value loaded before the loop
> >   // with the final one produced by the loop.
> >   fir.array_merge_store %array_a_dest_init, %array_a_final to %arg0 : !fir.array<?x!fir.type<_QFsTt{m:i32}>>, !fir.array<?x!fir.type<_QFsTt{m:i32}>>, !fir.box<!fir.array<?x!fir.type<_QFsTt{m:i32}>>>
> >   return
> > }
> > ```
> > 
> > This is an interesting example, and before even getting into the optimization section, it could be useful to explain how this works, and only then get into how it is optimized.
> > You introduced each operation individually, but seeing it all fit together is another story. 
> We can simplify and add more explanation to the example for sure. 
I update this section and kept just the example with annotation. I think it makes more sense to keep this documentation related to the array operations. Finer documentation on array-value-copy is probably better in the pass itself as comment or in a separate documentation. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115077