[PATCH] D113756: [fir] Add fir.embox conversion

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

I've only managed to skim through -  will revisit next wekk.

1. I think that the overall readability of CodeGen.cpp could be improved if `EmboxCommonConversion` was separate from the other conversions (which tend to me compact and self-contained). Perhaps a dedicated file?
2. `F18ADDENDUM` - it would be great to avoid `F18` (here and in other places). Perhaps `FLANG_ADDENDUM`?



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1209-1213
+  mlir::Value insertBaseAddress(mlir::ConversionPatternRewriter &rewriter,
+                                mlir::Location loc, mlir::Value dest,
+                                mlir::Value base) const {
+    return insertField(rewriter, loc, dest, {0}, base, /*bitCast=*/true);
+  }
----------------
Why not inline this?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1323
+                  mlir::ConversionPatternRewriter &rewriter) const override {
+    // There should be no dims on this embox op
+    assert(!embox.getShape());
----------------
1. Why?
2. Could you "inject" this comment into the `assert`?


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1218
+// CHECK:         %[[ELEM_SIZE:.*]] = llvm.mlir.constant(4 : i32) : i32
+// CHECK:         %[[TYPE_CODE:.*]] = llvm.mlir.constant(9 : i32) : i32
+// CHECK:         %[[I64_ELEM_SIZE:.*]] = llvm.sext %3 : i32 to i64
----------------
Where is `9` coming from? I couldn't find it.


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1219
+// CHECK:         %[[TYPE_CODE:.*]] = llvm.mlir.constant(9 : i32) : i32
+// CHECK:         %[[I64_ELEM_SIZE:.*]] = llvm.sext %3 : i32 to i64
+// CHECK:         %[[DESC0:.*]] = llvm.insertvalue %[[I64_ELEM_SIZE]], %[[DESC]][1 : i32] : !llvm.struct<(ptr<array<100 x i32>>, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}})>
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113756