[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/lib/Optimizer/CodeGen/CodeGen.cpp:2323
+        rewriter.create<mlir::LLVM::GEPOp>(
+            loc, mlir::LLVM::LLVMPointerType::get(llvmEleTy), args);
+      }
----------------
clementval wrote:
> awarzynski wrote:
> > 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 👍🏻 !
> Looks like you are making substantial change from the original code you are upstreaming. Might be wise to submit those change first in fir-dev to be reviewed and test with more extensive tests.
> Might be wise to submit those change first in fir-dev to be reviewed

I appreciate that you are concerned about the delta between `main` and `fir-dev`, but the code generated here and on `fir-dev` is identical (I've just pushed a small change to make sure that this condition is met). The biggest difference are the tests - these will be as "new" on `fir-dev` as they are here.

I'd like us to finish reviewing these changes here rather than on `fir-dev`. This is more inclusive (AFAIK, everyone from `fir-dev` is active here, but not everyone from `main` visits `fir-dev`). Perhaps sending a patch with tests added here to `fir-dev`  would be a good idea? The actual changes to `CoordinateOpConversion` would be sent later as a separate patch (i.e. as a "sync-up" patch). This way `fir-dev` developers can be confident that the changes introduced in `main` don't break the code. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114159