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

[Valentin Clement via Phabricator via llvm-commits]

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:
> 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112445