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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115077