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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini 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.
+
----------------
Can we link to a doc explaining this? (assuming this exists on the web, otherwise it would be worth an introduction)


================
Comment at: flang/docs/FIRArrayOperations.md:39
+syntax. `array_amend` annotates which loaded array copy is being written to.
+It is invalid to update an array copy without array_amend; doing so will result
+in undefined behavior.
----------------



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


================
Comment at: flang/docs/FIRArrayOperations.md:122
+
+It is only possible to use `array_fetch` on an `array_load` result value.
+
----------------
Why this restriction?


================
Comment at: flang/docs/FIRArrayOperations.md:138
+One can use `fir.array_update` to update the (implied) value of `a(i,j)`
+in an array expression as shown above.
+
----------------
Shouldn't the expression in Fortran above shows i and j  somehow?


================
Comment at: flang/docs/FIRArrayOperations.md:157
+
+The `array_access` operationis used to fetch the memory reference of an element
+in an array value.
----------------



================
Comment at: flang/docs/FIRArrayOperations.md:158
+The `array_access` operationis used to fetch the memory reference of an element
+in an array value.
+
----------------
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.


================
Comment at: flang/docs/FIRArrayOperations.md:180
+
+It is only possible to use `array_access` on an `array_load` result value.
+
----------------
Same question: why do we need this restriction?


================
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
----------------
You need to update the description of the "array type" to mention what this means.


================
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>
+```
+
----------------
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`?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115077