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

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


================
Comment at: flang/docs/FIRArrayOperations.md:18-19
+
+The array operations in FIR model the copy-in/copy-out semantics over Fortran
+statements.
+
----------------
mehdi_amini wrote:
> Can we link to a doc explaining this? (assuming this exists on the web, otherwise it would be worth an introduction)
We have this in the doc: https://github.com/flang-compiler/f18-llvm-project/blob/main/flang/docs/FortranForCProgrammers.md

We can probably add a small introduction here. 


================
Comment at: flang/docs/FIRArrayOperations.md:274
+  return
+}
+```
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115077