[PATCH] D112445: [fir] Add fir.array_access op

Tue Jan 25 10:15:59 PST 2022

clementval added inline comments.

================
Comment at: flang/include/flang/Optimizer/Dialect/FIROps.td:1570
+
+    It is only possible to use `array_access` on an `array_load` result value or
+    a value that can be trace back transitively to an `array_load` as the
----------------
mehdi_amini wrote:
> kiranchandramohan wrote:
> > clementval wrote:
> > > mehdi_amini wrote:
> > > > clementval wrote:
> > > > > mehdi_amini wrote:
> > > > > > kiranchandramohan wrote:
> > > > > > > clementval wrote:
> > > > > > > > kiranchandramohan wrote:
> > > > > > > > > Can this be verified in the verifier? Or is it too costly due to the transitive look up?
> > > > > > > > I think that it can be done. Do you mind if we do a separate patch for this?
> > > > > > > The reason I asked is that sometimes the verification checks which are expensive are in a pass.
> > > > > > > 
> > > > > > > That is OK with me. If no immediate plans then please create a ticket or let me know I will. It might be a good task for a beginner.
> > > > > > It's not clear to me that having such a structural requirement on the IR is a good idea in general: this makes it fragile to transformations in general and is quite concerning.
> > > > > This was discussed extensively in the array operations documentation patch. Just to make sure I understand your comment in the right way. Do you mean we should not verify such constraint or drop it? If we drop it, I guess the transformation that requires it will check for it and fails if it cannot verify this.
> > > > I think **requiring** this in the IR isn't a good design in itself and should be revisited. Anytime you make an SSA value not "standalone" but requiring to be structurally produced with specific constraints is breaking the very core definition of what is an SSA value.
> > > > Relying on such pattern to match for **optimization** purpose is one thing, but forcing it for correctness in the compiler is an entirely another can of worms.
> > > > 
> > > > We touched on it in the document patch: I still object to this. I don't think the design itself has really been properly reviewed and discussed upstream (as in: an RFC on discourse or on the development mailing-list).
> > > > (whatever happened downstream isn't very relevant to me).
> > > > 
> > > How different is than imposing a specific type? We could have a specific type derived from the array type we have currently and change the op definition to use this type instead of accepting all `fir_SequenceType`. In the end the restriction is pretty much similar or maybe I miss smth. 
> > > I agree that having such "restriction" hard-coded would make transformation quite difficult. In practice, right now, this is kind of an implied restriction since there is no other way to construct an meaninginful and "invalid" IR with those operations. 
> > The array operations are for restricted use and only for modelling the Fortran Array semantics. It helps lower the Array Expressions (to MLIR) and inserts temporaries wherever required. From what I understand the Array Value Copy pass will replace these array operations with other FIR operations and I believe this is one of the first passes (correct me If I am wrong) that will run. Hence I think any issue with the current design is localized to the pass and the array operations. So I was thinking it is OK to go ahead with the current design unless there are cases that will lead to incorrect code being generated.
> > 
> > I agree that we have not had RFC discussions about these topics. This is due to a lack of expertise (outside the FIR core team) and the development happening in the fir-dev repo. Since Fortran Array expressions are very important, if we pursue alternative designs without accepting the current design we will continue to have the unfortunate situation of two repos since I imagine this discussion will take a lot of time.
> > 
> > Since @mehdi_amini and @clementval have spent a lot of time reviewing and getting the patch ready, I am feeling uncomfortable submitting without further action or requesting action without submitting. 
> > 
> > I guess one action would be for @clementval and @schweitz to document the alternative approaches considered as well.
> > How different is than imposing a specific type? We could have a specific type derived from the array type we have currently and change the op definition to use this type instead of accepting all fir_SequenceType. In the end the restriction is pretty much similar or maybe I miss smth. 
> 
> It is very different: using a type does not require structurally that you can find the operation that produces it. For example it does not forbid to outline the code, or split the basic block and make it a basic block arguments.
> IIRC there has been issues with generic canonicalization for fir because of this kind of issues.
> 
> 
> 
Right. In this particular case the restriction does not prevent to split the block and make it a basic argument but it's through we had issue on another op. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112445