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

[Valentin Clement via Phabricator via llvm-commits]

clementval marked 7 inline comments as done.
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:
> > > mehdi_amini wrote:
> > > > mehdi_amini wrote:
> > > > > 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.
> > > > Another way to approach it is to analyze "what if this operand wasn't provided?" and answer the question with a description of how/where the optimizer or the codegen would be infeasible or very inconvenient.
> > > You pinged the revision but I don't think you actually followed up here, or did I miss something?
> > We have added some documentation in the last paragraph just below.
> OK, thanks.
> 
> I won't have bandwidth right now to page-in again all the context from back when I was actively trying to understand this in detail, so it's probably better if you just land this now.
Thanks for the reviews. The documentation can probably still be improved here and there and I guess it will be updated over time when other in the community get a chance to look at it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115077