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

[Valentin Clement via Phabricator via llvm-commits]

clementval 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>
----------------
awarzynski wrote:
> 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. 
We use `Lower` on at least 13 patterns in this file. Anyway I switched to conversion here. Not sure what do you want to "doxygen" here. 


================
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)
----------------
awarzynski wrote:
> This isn't correct, is it?
Why?


================
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);
----------------
awarzynski wrote:
> Why `1`?
Array size of 1


================
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 {
----------------
awarzynski wrote:
> Perhaps a daft question, but what's the difference between "descriptor" and "descriptor prefix" here?
Again, not the original author so just guessing. This is generating the first part of the descriptor (prefix) but not the last part with the dimensions and so on. 


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1285
+    dest = insertField(rewriter, loc, dest, {kF18AddendumPosInBox},
+                       this->genConstantOffset(loc, rewriter, hasAddendum));
+
----------------
awarzynski wrote:
> `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`. 
I split this. `genConstantOffset` is used mainly for GEP op creation and get the offset type from the TypeConverter. For constant created for values, we will use a separate function that just creates integer constants. 


================
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{{.*}})>
----------------
awarzynski wrote:
> 
Thanks!


================
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
----------------
awarzynski wrote:
> 
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113756