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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1668
+// CHECK:    %[[DERIVED_CAST_2:.*]] = llvm.bitcast %[[CAST_1]] : !llvm.ptr<i8> to !llvm.ptr<struct<"derived_2", (i32, i32)>>
+// CHECK:    %[[SUBOBJECT_2_ADDR:.*]] = llvm.getelementptr %[[DERIVED_CAST_2]][%[[C0_3]], %[[COORDINATE_2]]] : (!llvm.ptr<struct<"derived_2", (i32, i32)>>, i64, i32) -> !llvm.ptr<i32>
+// FIXME: These casts seem redundant
----------------
rovka wrote:
> This is very fishy, are you sure this is supposed to work? I'm following up to SUBOBJECT_1_ADDR, IIUC that's the address of field_1. It seems very bizarre to then take this ptr<i32> and cast it to <ptr<struct<"derived_2">> and index into it again. I feel like this should be an error, i.e. the thing that the second index is indexing into should be another struct or array or something, not an i32. Am I misunderstanding the coordinate_of semantics?
> This is very fishy
Agreed. Thanks for catching this and apologies for not paying more intention when submitting last night. I think that we should be getting the following instead (might be easier to parse without the regex):
```
  llvm.func @coordinate_box_derived_2(%arg0: !llvm.ptr<struct<(ptr<struct<"derived_2", (i32, i32)>>, i64, i32, i8, i8, i8, i8, ptr<i8>, array<1 x i64>)>>) {
    %0 = llvm.mlir.constant(0 : i32) : i32
    %1 = llvm.mlir.constant(1 : i32) : i32
    %2 = llvm.mlir.constant(0 : i32) : i32
    %3 = llvm.mlir.constant(0 : i32) : i32
    %4 = llvm.getelementptr %arg0[%2, %3] : (!llvm.ptr<struct<(ptr<struct<"derived_2", (i32, i32)>>, i64, i32, i8, i8, i8, i8, ptr<i8>, array<1 x i64>)>>, i32, i32) -> !llvm.ptr<ptr<struct<"derived_2", (i32, i32)>>>
    %5 = llvm.load %4 : !llvm.ptr<ptr<struct<"derived_2", (i32, i32)>>>
    %6 = llvm.mlir.constant(0 : i64) : i64
    %7 = llvm.getelementptr %5[%6, %0] : (!llvm.ptr<struct<"derived_2", (i32, i32)>>, i64, i32) -> !llvm.ptr<i32>
    %8 = llvm.getelementptr %5[%6, %1] : (!llvm.ptr<struct<"derived_2", (i32, i32)>>, i64, i32) -> !llvm.ptr<i32>
    %9 = llvm.bitcast %5 : !llvm.ptr<struct<"derived_2", (i32, i32)>> to !llvm.ptr<i32>
    llvm.return
  }
```
Does it make sense? I'll send an updated patch shortly. It gets rid of quite a few redundant casts. Unless I misunderstood GEP again and they are actually needed :) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114159