[flang-commits] [PATCH] D139020: [flang] hlfir.associate and hlfir.end_associate codegen

[Slava Zakharin via Phabricator via flang-commits]

vzakhari accepted this revision.
vzakhari added inline comments.
This revision is now accepted and ready to land.


================
Comment at: flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp:65
+static mlir::Value getBufferizedExprStorage(mlir::Value bufferizedExpr) {
+  auto tupleType = bufferizedExpr.getType().dyn_cast<mlir::TupleType>();
+  if (!tupleType)
----------------
jeanPerier wrote:
> vzakhari wrote:
> > It looks like there is an implicit assumption that there are no `mlir::TupleType` operands in the operations that are transformed by this pass before this pass launches OR if such operands are present and they match the `tuple<variable address, cleanupFlag>` pattern, then they indeed represent the bufferization package.  Am I reading it right?  It seems somewhat subtle to me, and I would rather use a distinct FIR type to represent the package so that values of this type have a well-defined meaning.  I guess it should be okay as-is if we do not expect `mlir::TupleType` operands ever appear elsewhere.
> There are no tuple operands before this pass. The operands of HLFIR ops have toe be hlfir.expr or HLFIR variable types, and tuple are not accepted by the verifiers. This full translation pass is translating `hlfir.expr<T>` types to `tuple<fir.ref/box<T>, i1>`. This helper is indeed relying on the fact that there are not other usage of tuple types.
> 
> I agree that introducing a new type would be nicer. But it would require adding ops to create/read them, and a pass to get rid of the type because convert-hlfir-to-fir cannot convert types (it is not a full rewrite pass to keep it simpler). So given the tuple creations and usages are internal to this file, I think it is OK to rely on the fact that other tuples cannot make it in this helpers.
> If it turns out we need something more solid because there is another use case for tuples as HLFIR operation operands, it will be OK to pay the price of adding a new type then.
> 
> I added a comment to make the assumption clear.
Thank you for the explanation and the comments!  It looks good to me.

Going forward, I think, we can try to keep using `fir.undefined` and `fir.insert_value` operations with the new type by extending `AnyCompositeLike` constraint.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139020