[PATCH] D113968: [fir] Add fircg.ext_array_coor conversion

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

I have commented inline about places where we seem to be using the original operands. Please have a look and if required, switch to using the converted operands.



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:141
+    auto dimValue = genConstantIndex(loc, idxTy, rewriter, dim);
+    return loadFromOffset(loc, box, c0, cDims, dimValue, 2, idxTy, rewriter);
+  }
----------------
Nit: 2 -> kDimStridePos ?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1817-1819
+      mlir::Value index = integerCast(loc, rewriter, idxTy, *indexOps);
+      mlir::Value lb =
+          isShifted ? integerCast(loc, rewriter, idxTy, *shiftOps) : one;
----------------
I think the indexOps and shiftOps are the original operands. We have to use the converted operands here I think.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1834
+      if (normalSlice) {
+        mlir::Value sliceLb = integerCast(loc, rewriter, idxTy, *sliceOps);
+        auto adj = rewriter.create<mlir::LLVM::SubOp>(loc, idxTy, sliceLb, lb);
----------------
Here also use converted.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1852
+        // (in element number).
+        auto nextExt = integerCast(loc, rewriter, idxTy, *shapeOps);
+        prevExt =
----------------
Use converted in place of shapeOps.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1934
+    SmallVector<mlir::Value> args = {base, offset};
+    args.append(coor.subcomponent().begin(), coor.subcomponent().end());
+    rewriter.replaceOpWithNewOp<mlir::LLVM::GEPOp>(coor, ty, args);
----------------
This should be the converted operands and not the original.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1801-1804
+    auto indexOps = coor.indices().begin();
+    auto shapeOps = coor.shape().begin();
+    auto shiftOps = coor.shift().begin();
+    auto sliceOps = coor.slice().begin();
----------------
clementval wrote:
> kiranchandramohan wrote:
> > Nit: Since these are all for the `for` loop, would it be better to initialize it in the loop init section?
> This doesn't work AFAIK or am I missing something?
OK. Can you move it closer (just above) to the for loop?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113968