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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski marked 6 inline comments as done.
awarzynski added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:71
   }
+  mlir::Type voidPtrTy() const {
+    return getVoidPtrType(&lowerTy().getContext());
----------------
rovka wrote:
> Is this actually used?
Nope. Good catch, thanks!


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:2249
+  int64_t getIntValue(mlir::Value val) const {
+    if (val) {
+      if (auto *defop = val.getDefiningOp()) {
----------------
rovka wrote:
> 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.
Thanks for the suggestions! Let me rewrite this. Stay tuned!


================
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
----------------
rovka wrote:
> 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)
Good catch, thanks! This is currently not being captured (so can't a test in "invalid.fir"). I'll take a look later and see whether I can implement a separate verifier for this. Probably in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114159