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

Mon Jan 24 23:30:11 PST 2022

mehdi_amini 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
----------------
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).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112445