[PATCH] D114709: [Flang] Upstream conversion of the XRebox Op

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:194
+  static mlir::Type getBaseAddrTypeFromBox(mlir::Type type) {
+    return getBoxEleTy(type, {0});
+  }
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1461
+                               unsigned dim, mlir::Value lb) const {
+    return insertField(rewriter, loc, dest, {7, dim, 0}, lb);
+  }
----------------
I introduced some constexpr for the different element in `dim` in this patch https://reviews.llvm.org/D114148#change-7X7u66OPbYbl but it is not in yet. 

Feel free to base your patch on mine or just bring in these 3 constexpr so the magic number will have a meaning. 
```
static constexpr unsigned kDimLowerBoundPos = 0;
static constexpr unsigned kDimExtentPos = 1;
static constexpr unsigned kDimStridePos = 2;
```


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1466
+                           mlir::Value extent) const {
+    return insertField(rewriter, loc, dest, {7, dim, 1}, extent);
+  }
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1471
+                           mlir::Value stride) const {
+    return insertField(rewriter, loc, dest, {7, dim, 2}, stride);
+  }
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1721
+    auto loc = rebox.getLoc();
+    auto one = genConstantIndex(loc, lowerTy().indexType(), rewriter, 1);
+    llvm::outs() << "extents = " << extents.size()
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1744-1745
+    auto loc = rebox.getLoc();
+    auto voidPtrTy = getVoidPtrType(rebox.getContext());
+    auto idxTy = lowerTy().indexType();
+    mlir::Value zero = genConstantIndex(loc, idxTy, rewriter, 0);
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1779
+    llvm::SmallVector<mlir::Value> slicedStrides;
+    auto one = genConstantIndex(loc, idxTy, rewriter, 1);
+    const bool sliceHasOrigins = !rebox.shift().empty();
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1781-1784
+    auto sliceOps = rebox.sliceOffset();
+    auto shiftOps = rebox.shiftOffset();
+    auto strideOps = inputStrides.begin();
+    const auto inputRank = inputStrides.size();
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1787
+         ++i, ++strideOps, ++shiftOps, sliceOps += 3) {
+      auto sliceLb = integerCast(loc, rewriter, idxTy, operands[sliceOps]);
+      auto inputStride = *strideOps; // already idxTy
----------------
kiranchandramohan wrote:
> @jeanPerier  Here and in a few other places we were using the original operands and not the converted operands. This caused some issue when I cherry-picked the patch to llvm-project/flang. In discourse it was pointed out that this is because we are not using the converted operands (https://llvm.discourse.group/t/issue-in-conversion-from-index-to-i64/4853). Is there any reason we were using the original operands? 
> For getting the patch to work I had to switch to the converted operands.



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1790
+      // Apply origin shift: base += (lb-shift)*input_stride
+      auto sliceOrigin = sliceHasOrigins ? integerCast(loc, rewriter, idxTy,
+                                                       operands[shiftOps])
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1804
+      if (isTripletSlice) {
+        auto step = integerCast(loc, rewriter, idxTy, operands[sliceOps + 2]);
+        // extent = ub-lb+step/step
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1806
+        // extent = ub-lb+step/step
+        auto sliceUb = integerCast(loc, rewriter, idxTy, upper);
+        auto extent = computeTripletExtent(rewriter, loc, sliceLb, sliceUb,
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1807
+        auto sliceUb = integerCast(loc, rewriter, idxTy, upper);
+        auto extent = computeTripletExtent(rewriter, loc, sliceLb, sliceUb,
+                                           step, zero, idxTy);
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1838
+    // Strides from the fir.box are in bytes.
+    auto voidPtrTy = getVoidPtrType(rebox.getContext());
+    base = rewriter.create<mlir::LLVM::BitcastOp>(loc, voidPtrTy, base);
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1843
+    llvm::SmallVector<mlir::Value> newExtents;
+    auto idxTy = lowerTy().indexType();
+    // First stride from input box is kept. The rest is assumed contiguous
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114709