[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: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:
> > > > > > 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?
> It's useful because it bounds the second of the FIR that represents the statements like a `FORALL`. Statement have specific semantics and like I mentioned before it is useful for analysis for example to know the "bound" defined by array_load and array_merge_store. 
I feel we're going in rounds here, you keep telling it is useful and I keep asking you to elaborate on this: can you put it in the doc.
A reader of the doc shouldn't have to "guess" the design and a design doc should go beyond explaining just what it does but also capture the "why" it is done this way.
If you say "it is useful", then please demonstrate it, or illustrate with examples.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115077