[PATCH] D114535: [fir] Add fir ragged array builder

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

I have a couple of questions.



================
Comment at: flang/lib/Optimizer/Builder/Runtime/Ragged.cpp:35
+  auto ptrTy = builder.getRefType(eleTy.cast<mlir::TupleType>().getType(1));
+  auto ptr = builder.create<fir::CoordinateOp>(loc, ptrTy, header, one);
+  auto heap = builder.create<fir::LoadOp>(loc, ptr);
----------------
I guess this is two now, right?


================
Comment at: flang/unittests/Optimizer/Builder/Runtime/RaggedTest.cpp:19
+      fir::SequenceType::get(fir::SequenceType::Shape(1, 10), tupleTy);
+  mlir::Value header = firBuilder->create<fir::UndefOp>(loc, seqTy);
+  mlir::Value eleSize = firBuilder->createIntegerConstant(loc, i32Ty, 1);
----------------
Nit: I guess this is not a true representation of a header.
But I guess it is OK since we are not testing the structure of the header.

Also, is the tupleTy/pair due to the previous representation of the raggedArrayHeader?


================
Comment at: flang/unittests/Optimizer/Builder/Runtime/RaggedTest.cpp:32
+      fir::SequenceType::get(fir::SequenceType::Shape(1, 10), i32Ty);
+  mlir::Value header = firBuilder->create<fir::UndefOp>(loc, seqTy);
+  fir::runtime::genRaggedArrayDeallocate(loc, *firBuilder, header);
----------------
Nit: This seems to be different from the header creation above.

I guess a common getHeader function to get the header and a comment indicating that this is not a true header but just used for testing the call to the Ragged runtime functions might be helpful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114535