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

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2323
+        rewriter.create<mlir::LLVM::GEPOp>(
+            loc, mlir::LLVM::LLVMPointerType::get(llvmEleTy), args);
+      }
----------------
awarzynski wrote:
> 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?
If you are confident that your change will not trigger a regression on fir-dev then fine for me. The change seems bigger than it really is. I was seeing something about fixing this conversion so I was just worried about what had to be fixed.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114159