[PATCH] D112845: [fir] Add base of the FIR to LLVM IR pass

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: flang/include/flang/Optimizer/CodeGen/CGPasses.td:28
+    "fir::FIROpsDialect", "fir::FIRCodeGenDialect", "mlir::BuiltinDialect",
+    "mlir::LLVM::LLVMDialect", "mlir::omp::OpenMPDialect"
+  ];
----------------
Can you revisit the dependentDialects list?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:47-49
+} // namespace
+
+namespace {
----------------
delete? (closing/reopening the anonymous namespace)


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:74
+
+struct GlobalOpConversion : public FIROpConversion<fir::GlobalOp> {
+  using FIROpConversion::FIROpConversion;
----------------
I think it'd be great to document each of the conversion if possible


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:95
+      // Replace insert_on_range with a constant dense attribute if the
+      // initialization is on the full range.
+      auto insertOnRangeOps = gr.front().getOps<fir::InsertOnRangeOp>();
----------------
Isn't this something that should be a `fold()` on the insert_on_range op instead?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:147
+        return mlir::LLVM::Linkage::Weak;
+    }
+    return mlir::LLVM::Linkage::External;
----------------
String comparison is... sad.
Can you add TODO to update the linkage on the fir global to be an enum instead?
(you may want to reuse the LLVM::Linkage?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112845