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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1045
 
+// Common base class for lowering of embox to descriptor creation.
+template <typename OP>
----------------
DOXYGENME?

[nit] IIUC, `lowering` is used in Flang to refer to lowering from parse tree to FIR. This part is usually referred to as "code-gen". And this file deals with "conversions" specifically. Perhaps "lowering" --> "converting"? For the sake of consistency. 


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1055-1056
+    auto thisPt = rewriter.saveInsertionPoint();
+    auto *thisBlock = rewriter.getInsertionBlock();
+    auto op = thisBlock->getParentOp();
+    // Order to find the Op in whose entry block the alloca should be inserted.
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1058-1059
+    // Order to find the Op in whose entry block the alloca should be inserted.
+    // The parent Op if it is an LLVM Function Op.
+    // The ancestor LLVM Function Op.
+    auto func = mlir::isa<mlir::LLVM::LLVMFuncOp>(op)
----------------
This isn't correct, is it?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1064
+    rewriter.setInsertionPointToStart(&func.front());
+    auto sz = this->genConstantOffset(loc, rewriter, 1);
+    auto al = rewriter.create<mlir::LLVM::AllocaOp>(loc, toTy, sz, alignment);
----------------
Why `1`?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1070-1077
+  int getCFIAttr(fir::BoxType boxTy) const {
+    auto eleTy = boxTy.getEleTy();
+    if (eleTy.isa<fir::PointerType>())
+      return CFI_attribute_pointer;
+    if (eleTy.isa<fir::HeapType>())
+      return CFI_attribute_allocatable;
+    return CFI_attribute_other;
----------------
I think that this can be `static`, no?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1256
+  std::tuple<fir::BoxType, mlir::Value, mlir::Value>
+  consDescriptorPrefix(BOX box, mlir::ConversionPatternRewriter &rewriter,
+                       unsigned rank, mlir::ValueRange lenParams) const {
----------------
Perhaps a daft question, but what's the difference between "descriptor" and "descriptor prefix" here?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1285
+    dest = insertField(rewriter, loc, dest, {kF18AddendumPosInBox},
+                       this->genConstantOffset(loc, rewriter, hasAddendum));
+
----------------
`genConstantOffset` takes an `int` rather than ` bool`. 

Also, `hasAddendum`  is not an offset, is it?. Similarly above (`getCFIAttr` does not generate an offset, does it). In general, based on the usage in this method, it seems that `genConstantOffset` should be renamed as `genConstant`. 


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1206
+// Check basic creation of a descriptor and insertion of values.
+
+func @embox0(%arg0: !fir.ref<!fir.array<100xi32>>) {
----------------
Could add a note explaining the origins of the indices within the descriptor?


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1229
+// CHECK:         %[[ATTR:.*]] = llvm.mlir.constant(0 : i32) : i32
+// CHECK:         %[[ATTR_I8:.*]] = llvm.trunc %14 : i32 to i8
+// CHECK:         %[[DESC4:.*]] = llvm.insertvalue %15, %[[DESC3]][5 : i32] : !llvm.struct<(ptr<array<100 x i32>>, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}})>
----------------



================
Comment at: flang/test/Fir/convert-to-llvm.fir:1230
+// CHECK:         %[[ATTR_I8:.*]] = llvm.trunc %14 : i32 to i8
+// CHECK:         %[[DESC4:.*]] = llvm.insertvalue %15, %[[DESC3]][5 : i32] : !llvm.struct<(ptr<array<100 x i32>>, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}})>
+// CHECK:         %[[F18ADDENDUM:.*]] = llvm.mlir.constant(0 : i32) : i32
----------------



================
Comment at: flang/test/Fir/convert-to-llvm.fir:1253
+// Check `fir.embox` conversion of a POINTER entity. Make sure that the
+// attribute in the descriptor is set to 1.
+
----------------
Could you explain "why" `1`? (i.e. that's the value of `CFI_attribute_pointer`, right?). 


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1268
+// Check `fir.embox` conversion of an ALLOCATABLE entity. Make sure that the
+// attribute in the descriptor is set to 2.
+
----------------
Why `2`?


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1276
+// CHECK-LABEL: llvm.func @embox_allocatable
+// CHECK: %[[CFI_ATTR_POINTER:.*]] = llvm.mlir.constant(2 : i32) : i32
+// CHECK: %[[ATTR_I8:.*]] = llvm.trunc %[[CFI_ATTR_POINTER]] : i32 to i8
----------------



================
Comment at: flang/test/Fir/convert-to-llvm.fir:1329
+// Check descriptor for a derived type. Check that the f18Addendum flag is set
+// to 1 and the addendum values are inserted.
+
----------------
Why `1`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113756