[PATCH] D114159: [flang][codegen] Add a conversion for `fir.coordinate_of` - part 1

[Diana Picus via Phabricator via llvm-commits]

rovka added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:71
   }
+  mlir::Type voidPtrTy() const {
+    return getVoidPtrType(&lowerTy().getContext());
----------------
Is this actually used?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2249
+  int64_t getIntValue(mlir::Value val) const {
+    if (val) {
+      if (auto *defop = val.getDefiningOp()) {
----------------
Nit: I think you can flip this function around (i.e. check the preconditions first and have the bulk of the code outside of the ifs).
Also, I'm almost afraid to suggest this, but I think the unreachable can be replaced with an assert.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2296
+      unsigned dims = arrTy.getDimension();
+      assert(operands.size() - 1 == dims && "Wrong number of indices");
+      // Applies byte strides from the box. Ignore lower bound from box
----------------
Not saying it should be part of this patch, but is there a test for this somewhere? (I'd expect it in test/Fir/invalid.fir, but AFAICT there isn't anything there)


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1626
+  %idx = arith.constant 1 : i32
+  %q = fir.coordinate_of %arg0, %idx : (!fir.box<!fir.type<derived_1{field_1:i32, field_2:i32}>>, i32) -> !fir.ref<i32>
+  return
----------------
Would be nice to also have a testcase with more than one index operand.


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1634
+// CHECK:    %[[C0_2:.*]] = llvm.mlir.constant(0 : i32) : i32
+// CHECK:    %[[DERRIVED_ADDR:.*]] = llvm.getelementptr %[[BOX]][%[[C0_1]], %[[C0_2]]] : (!llvm.ptr<struct<(ptr<struct<"derived_1", (i32, i32)>>, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, ptr<i8>, array<1 x i64>)>>, i32, i32) -> !llvm.ptr<ptr<struct<"derived_1", (i32, i32)>>>
+// CHECK:    %[[DERRIVED_VAL:.*]] = llvm.load %[[DERRIVED_ADDR]] : !llvm.ptr<ptr<struct<"derived_1", (i32, i32)>>>
----------------



================
Comment at: flang/test/Fir/convert-to-llvm.fir:1635
+// CHECK:    %[[DERRIVED_ADDR:.*]] = llvm.getelementptr %[[BOX]][%[[C0_1]], %[[C0_2]]] : (!llvm.ptr<struct<(ptr<struct<"derived_1", (i32, i32)>>, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, ptr<i8>, array<1 x i64>)>>, i32, i32) -> !llvm.ptr<ptr<struct<"derived_1", (i32, i32)>>>
+// CHECK:    %[[DERRIVED_VAL:.*]] = llvm.load %[[DERRIVED_ADDR]] : !llvm.ptr<ptr<struct<"derived_1", (i32, i32)>>>
+// CHECK:    %[[C0_3:.*]] = llvm.mlir.constant(0 : i64) : i64
----------------



================
Comment at: flang/test/Fir/convert-to-llvm.fir:1638
+// FIXME: This cast seems redundant
+// CHECK:    %[[DERRIVED_CAST:.*]] = llvm.bitcast %[[DERRIVED_VAL]] : !llvm.ptr<struct<"derived_1", (i32, i32)>> to !llvm.ptr<struct<"derived_1", (i32, i32)>>
+// CHECK:    %[[SUBOBJECT_ADDR:.*]] = llvm.getelementptr %[[DERRIVED_CAST]][%[[C0_3]], %[[COORDINATE]]] : (!llvm.ptr<struct<"derived_1", (i32, i32)>>, i64, i32) -> !llvm.ptr<i32>
----------------



================
Comment at: flang/test/Fir/convert-to-llvm.fir:1640
+// CHECK:    %[[SUBOBJECT_ADDR:.*]] = llvm.getelementptr %[[DERRIVED_CAST]][%[[C0_3]], %[[COORDINATE]]] : (!llvm.ptr<struct<"derived_1", (i32, i32)>>, i64, i32) -> !llvm.ptr<i32>
+// FIXMR: These casts seem redundant
+// CHECK:    %[[CAST_1:.*]] = llvm.bitcast %[[SUBOBJECT_ADDR]] : !llvm.ptr<i32> to !llvm.ptr<i8>
----------------



================
Comment at: flang/test/Fir/convert-to-llvm.fir:1654
+// CHECK-SAME:  %[[COORDINATE:.*]]: i64
+// There's only one box here. It's index is `0`. Generate it.
+// CHECK-NEXT:  %[[BOX_IDX:.*]] = llvm.mlir.constant(0 : i32) : i32
----------------



================
Comment at: flang/test/Fir/convert-to-llvm.fir:1685
+// CHECK-SAME: %[[COORDINATE_1:.*]]: i64, %[[COORDINATE_2:.*]]: i64)
+// There's only one box here. It's index is `0`. Generate it.
+// CHECK-NEXT:  %[[BOX_IDX:.*]] = llvm.mlir.constant(0 : i32) : i32
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114159