[PATCH] D118436: [flang] Initial lowering for empty program

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

Yaay, it's great to see this patch! :) 
I think it's a bit large though, maybe we can remove some things that aren't strictly necessary to make the test pass? That would make both this patch and future patches (where things are actually useed) easier to review.



================
Comment at: flang/include/flang/Lower/CallInterface.h:80
+/// how the interface must be. Then, the funcOp is created for it. Then a simple
+/// pass over fir arguments finalize the interface information that must be
+/// passed back to the user (and may require having the funcOp). All this
----------------



================
Comment at: flang/include/flang/Lower/CallInterface.h:81
+/// pass over fir arguments finalize the interface information that must be
+/// passed back to the user (and may require having the funcOp). All this
+/// passes are driven from the CallInterface constructor.
----------------



================
Comment at: flang/include/flang/Lower/CallInterface.h:115
+    /// Position of related passedEntity in passedArguments.
+    /// (passedEntity is the passedResult this value is resultEntityPosition.
+    int passedEntityPosition;
----------------
Nit: missing ).
Also, I can't really guess what this comment means since I haven't read the whole patch yet and have no idea what all these names refer to. Maybe it would be a good idea to clarify the expected usage of FirPlaceHolder?


================
Comment at: flang/include/flang/Lower/CallInterface.h:119
+    /// through this argument.
+    Property property;
+    /// MLIR attributes for this argument
----------------
Why do we need this? Wouldn't this be obvious from the type? (Maybe add it when it's actually used, then hopefully it will be obvious?)


================
Comment at: flang/include/flang/Lower/CallInterface.h:136
+  llvm::SmallVector<FirPlaceHolder> outputs;
+  llvm::SmallVector<FirPlaceHolder> inputs;
+  mlir::FuncOp func;
----------------
Are these actually written in this patch? Or just read? Can we introduce them later?


================
Comment at: flang/lib/Lower/Bridge.cpp:39
+/// Traverse the pre-FIR tree (PFT) to generate the FIR dialect of MLIR.
+class FirConverter : public Fortran::lower::AbstractConverter {
+public:
----------------
I'm seeing a lot of final overrides, is the whole class intended to be final by any chance? If so, we should just mark it as such upfront.


================
Comment at: flang/lib/Lower/Bridge.cpp:150
+
+  fir::FirOpBuilder &getFirOpBuilder() override final { return *builder; }
+
----------------
Might crash and burn if builder isn't instantiated yet.


================
Comment at: flang/lib/Lower/Bridge.cpp:188
+  void startNewFunction(Fortran::lower::pft::FunctionLikeUnit &funit) {
+    assert(!builder && "expected nullptr");
+    Fortran::lower::CalleeInterface callee(funit, *this);
----------------
Nit: Should we also assert that localSymbols is empty?


================
Comment at: flang/lib/Lower/Bridge.cpp:193
+    assert(builder && "FirOpBuilder did not instantiate");
+    builder->setInsertionPointToStart(&func.front());
+    func.setVisibility(mlir::SymbolTable::Visibility::Public);
----------------
Nit: Changing the visibility doesn't seem to be related to the builder, so maybe move it up right after the creation of func? And then you can probably remove the changes to the insertion point from this patch and upstream them when they're actually relevant.


================
Comment at: flang/lib/Lower/Bridge.cpp:249
+  fir::FirOpBuilder *builder = nullptr;
+  Fortran::lower::pft::Evaluation *evalPtr = nullptr;
+  Fortran::lower::SymMap localSymbols;
----------------
Nit: Is this actually used?


================
Comment at: flang/lib/Lower/Bridge.cpp:250
+  Fortran::lower::pft::Evaluation *evalPtr = nullptr;
+  Fortran::lower::SymMap localSymbols;
+  Fortran::parser::CharBlock currentPosition;
----------------
Where is this used? I see some references to it, but it doesn't seem to be populated (or I could be just missing it, it's Friday). Maybe we can upstream the SymMap in a future patch, to make this easier to review?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118436