[PATCH] D114148: [fir] Add fircg.ext_embox conversion

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1791
+      if (!xbox.substr().empty())
+        base = shiftSubstringBase(rewriter, loc, base, xbox.substr()[0]);
+    }
----------------
I think the last argument is using the original operand and not the converted operand.


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1577
+
+// Check complete `fircg.ext_embox`.
+
----------------
Nit: Consider this as a nit and I will leave it to you to decide whether we should have it in this patch or should do it later.

1. A more realistic test case will be helpful here. Consider the following, it has shape, slice and shift operands for the op. Since it has two dimensions, it will exercise the for loop inside the code.

```
// subroutine sb(n,sh1,sh2)
// integer::n,sh1,sh2
// double precision::arr(sh1:n,sh2:n)
// call xb(arr(2:n,4:n))
// end subroutine
```

```
! N is the upperbound, sh1 and sh2 are the shifts or lowerbounds
func @_QPsb(%N: index, %sh1: index, %sh2: index) {
  %c4 = arith.constant 4 : index
  %c1 = arith.constant 1 : index
  %c2 = arith.constant 2 : index
! Calculate nelems in dim1
  %n1_tmp = arith.subi %N, %sh1 : index
  %n1 = arith.addi %n1_tmp, %c1 : index
! Calculate nelems in dim2
  %n2_tmp = arith.subi %N, %sh2 : index
  %n2 = arith.addi %n2_tmp, %c1 : index
  %arr = fir.alloca !fir.array<?x?xf64>, %n1, %n2 {bindc_name = "arr", uniq_name = "_QFsbEarr"}
  %box = fircg.ext_embox %arr(%n1, %n2) origin %sh1, %sh2[%c2, %N, %c1, %c4, %N, %c1] : (!fir.ref<!fir.array<?x?xf64>>, index, index, index, index, index, index, index, index, index, index) -> !fir.box<!fir.array<?x?xf64>>
  fir.call @_QPxb(%box) : (!fir.box<!fir.array<?x?xf64>>) -> ()
  return
}      
func private @_QPxb(!fir.box<!fir.array<?x?xf64>>)
```
2. A test with a subcomponent if possible


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1581
+  %c0 = arith.constant 0 : i64
+  %0 = fircg.ext_embox %arg0(%c0) origin %c0[%c0, %c0, %c0] substr %c0, %c0 : (!fir.ref<!fir.array<?xi32>>, i64, i64, i64, i64, i64, i64, i64) -> !fir.box<!fir.array<?xi32>>
+  return
----------------
Does substr have a meaning here since it applies to chars?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114148