[PATCH] D80449: [flang] Upstream changes to the PFT data structure

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Thanks @schweitz for submitting this change. I also felt that putting out all the changes in PFTBuilder as a single step is the best way forward while preparing D79731 <https://reviews.llvm.org/D79731>. I guess this will simplify the introduction of the bridge code.

I see that bulk of the changes are in PFTBuilder.h and PFTBuilder.cpp. Many are cosmetic but some changes look like a design change? I have summarised below the changes that I saw just for reference. It will be great to have some more explanation for the key changes in the code or in the commit message. Particularly for the introduction of the ReferenceVariant, removal of the annote functions, CFGannotations enum and the introduction of analyse functions.

Also, should any of the new pointers introduced in Evaluation/PFTBuilder struct/class be smart pointers?

PFTBuilder.h
-> Rename
EvaluationCollection to EvaluationList
ParentType to ParentVariant
Split constructs into constructs and directives.
FunctionLikeUnit:
evals -> evaluationList
funcs -> nestedFunctions (also in Module like unit)

-> Modification
createPFT function takes another argument semanticsContext

-> Introduce
LabelEvalMap type (an llvm dense map from Label to Evaluation*)
isIntermediateConstructStmt, isNopConstructStmt template variable expressions.
Types LabelSet, SymbolRef, SymbolLabelMap
ReferenceVariant class and helpers.
EvaluationTuple, EvaluationVariant, struct Evaluation
Struct variable
FunctionLikeUnit:
Functions to get location
LabelEvalMap labelEvaluationMap;
SymbolLabelMap assignSymbolLabelMap;
const semantics::Symbol *symbol{nullptr};
mlir::Block *finalBlock{};
std::vector<std::vector<Variable>> varList;

-> Remove
CFGAnnotation
CGJump
annotateControlFunction

PFTBuilder.cpp
-> Rename
All names with Eval changed to Evaluation. (dumpEvalList -> dumpEvaluationList)
evallist -> evaluationListStack
parents -> parentVariantStack

-> Introduce
getConstructName function
struct SymbolDependenceDepth and processSymbolTable function
const semantics::SemanticsContext &semanticsContext;
std::vector<lower::pft::Evaluation *> constructAndDirectiveStack{};
std::vector<lower::pft::Evaluation *> doConstructStack{};
llvm::DenseMap<parser::Label, lower::pft::Evaluation *> *labelEvaluationMap{nullptr};
lower::pft::SymbolLabelMap *assignSymbolLabelMap{nullptr};
std::map<std::string, lower::pft::Evaluation *> constructNameMap{};
lower::pft::Evaluation *lastLexicalEvaluation{nullptr};

-> Modification
analyzeBranches kind of replaces the annotate* functions. It  is also called during PFT construction and hence need not be called separately, hence the call to annotateControl is not needed from f18.cpp.

Pre-fir tree tests
Minor change in tests {introduces space, distinguishes structured and unstructured}



================
Comment at: flang/include/flang/Lower/PFTBuilder.h:46
 
-struct ParentType {
+struct ParentVariant {
   template <typename A>
----------------
Why is this not a reference variant?


================
Comment at: flang/include/flang/Lower/PFTBuilder.h:134
+
+/// Provide a variant like container that can holds constant references.
+/// It is used in the other classes to provide union of const references
----------------
nit: holds->hold


================
Comment at: flang/include/flang/Lower/PFTBuilder.h:336
+  bool global;
+  //bool heap{false}; // variable needs deallocation on exit
 };
----------------
nit: remove this?


================
Comment at: flang/lib/Lower/PFTBuilder.cpp:1044
+    sdd.analyze(iter.second.get());
+    // llvm::outs() << iter.second.get() << '\n';
+  }
----------------
nit: Remove this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80449