[PATCH] D112207: [fir] Add utility function to FIRBuilder and MutableBox

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.
This revision is now accepted and ready to land.

LGTM. I have a few minor/trivial comments or questions.



================
Comment at: flang/include/flang/Optimizer/Builder/MutableBox.h:126
+/// and is-up to date, this is a no-op, otherwise, code will be generated to
+/// fill the it.
+mlir::Value getMutableIRBox(fir::FirOpBuilder &builder, mlir::Location loc,
----------------
Nit: fill the it -> fill it? Or something else is missing.


================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:292
+  auto shapeType = fir::ShapeShiftType::get(getContext(), exts.size());
+  llvm::SmallVector<mlir::Value> shapeArgs;
+  auto idxTy = getIndexType();
----------------
Nit:Can probably size this to exts.size()*2?


================
Comment at: flang/lib/Optimizer/Builder/MutableBox.cpp:90
+  readShape(llvm::SmallVectorImpl<mlir::Value> *lbounds = nullptr) {
+    llvm::SmallVector<mlir::Value> extents;
+    auto rank = box.rank();
----------------
Nit: might be possible to set the size before entering the loop.


================
Comment at: flang/lib/Optimizer/Builder/MutableBox.cpp:326
+  // Provide dummy length parameters if they are dynamic. If a length parameter
+  // is deferred. it is set to zero here and will be set on allocation.
+  llvm::SmallVector<mlir::Value> lenParams;
----------------
Nit: it -> It


================
Comment at: flang/lib/Optimizer/Builder/MutableBox.cpp:357
+
+/// Helper to decide if a MutableBoxValue must be read to an BoxValue or
+/// can be read to a reified box value.
----------------
Nit: an BoxValue -> a BoxValue


================
Comment at: flang/lib/Optimizer/Builder/MutableBox.cpp:369
+    return true;
+  // Intrinsic alloctables are contiguous, no need to track the value by
+  // fir.box.
----------------
Nit: alloctables -> allocatables


================
Comment at: flang/lib/Optimizer/Builder/MutableBox.cpp:373
+    return false;
+  // Pointer are known to be contiguous at compile time iff they have the
+  // CONTIGUOUS attribute.
----------------
Nit: Pointer -> Pointers


================
Comment at: flang/lib/Optimizer/Builder/MutableBox.cpp:735
+/// MutableBoxValue need to be synced before and after calls passing the
+/// descriptor. These calls will generate the syncing if needed and be no-op
+mlir::Value fir::factory::getMutableIRBox(fir::FirOpBuilder &builder,
----------------
Nit: Is something missing towards the end?


================
Comment at: flang/unittests/Optimizer/Builder/FIRBuilderTest.cpp:360
+
+TEST_F(FIRBuilderTest, genShapeWithAbstractArrayBox) {
+  auto builder = getBuilder();
----------------
Is the second version of genShape tested indirectly through this? 
Might be good to have something with shapeshift. Or does testing that require codegen?


================
Comment at: flang/unittests/Optimizer/Builder/FIRBuilderTest.cpp:381
+
+TEST_F(FIRBuilderTest, getExtents) {
+  auto builder = getBuilder();
----------------
Can other variants (CharArrayBox,MutableArrayBox) be tested? Or do they require codegen?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112207