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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.

I have a few Nit comments. Otherwise LGTM.



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


================
Comment at: flang/lib/Optimizer/Dialect/FIROps.cpp:505
+  auto arrTy = op.sequence().getType().cast<fir::SequenceType>();
+  auto indSize = op.indices().size();
+  if (indSize < arrTy.getDimension())
----------------
Nit: Remove auto


================
Comment at: flang/lib/Optimizer/Dialect/FIROps.cpp:511
+    return op.emitOpError("return type does not match array");
+  auto ty = validArraySubobject(op);
+  if (!ty || fir::ReferenceType::get(ty) != op.getType())
----------------
Nit: Remove auto


================
Comment at: flang/lib/Optimizer/Dialect/FIROps.cpp:513
+  if (!ty || fir::ReferenceType::get(ty) != op.getType())
+    return op.emitOpError("return type and/or indices do not type check");
+  return mlir::success();
----------------
Nit: Can you add a test for this check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112445