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

Mon Nov 29 10:31:43 PST 2021

awarzynski added inline comments.

================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2323
+        rewriter.create<mlir::LLVM::GEPOp>(
+            loc, mlir::LLVM::LLVMPointerType::get(llvmEleTy), args);
+      }
----------------
clementval wrote:
> rovka wrote:
> > This still doesn't seem quite right. How are all these GEPs used? It seems you're just creating them, but then you're replacing the original coor with objectBaseAddr, so you're just going around the GEPs (i.e. the users of the coor will get objectBaseAddr instead of a GEP based on objectBaseAddr).
> > 
> Please double check in `fir-dev` when you change code like that. 
@rovka 

Good point, thanks! Also, your comment made me realise that I misunderstood how `fir.coordinate_of` is meant to work for `fir.type` and `fir.array`. In particular, `fir.type` could contain `fir.array` and vice versa. This is catered for on `fir-dev` (see [[ https://github.com/flang-compiler/f18-llvm-project/blob/fir-dev/flang/lib/Optimizer/CodeGen/CodeGen.cpp#L2158-L2207 | here ]] ), but was incorrectly split into "orthogonal" cases by me in this patch. I will revert that and restore one big case for `fir.type`/`fir.array` instead. 

And yes, this "rewrite" should replace the original OP with a GEP. In theory :) There's a bunch of bitcasts too as `fir.array` and `fir.type` cases are a bit different. So, ultimately, it might be an `llvm.bitcast` applied to a result of `llvm.getelemenptr`. 

> Please double check in fir-dev when you change code like that.

Checked, all good 👍🏻 !

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114159