[flang][OpenMP][NFC] Refactor to avoid global variable

[tblah]

I was really hoping this would also work for hostEvalInfo but unfortunately that needed to be shared to a greater degree.

The same technique should work for that but it would need that class to be made public and then the state kept between calls to genOpenMP*Construct, which felt like more trouble than it was worth.

I'm open to abandoning this patch if solving one global variable doesn't feel worth this much churn.

Making these changes I was wondering if we should implement this file with one big class to wrap up all the state passed to every function. Any thoughts?

[skatrak]

Thank you Tom for taking a stab at this issue. In my opinion, we should probably take this opportunity to address it for both hostEvalInfo and sectionsStack in a slightly more generic way.

I think you're referring to this yourself in the "FIXME" comment you added for hostEvalInfo, but in my opinion it would be the AbstractConverter class the one that should hold a stack of generic information and provide basic facilities to introduce, query and remove objects from it. That way, we can produce information on parent constructs that can then be queried by their children, which would address the current need for any global variables here.

The ModuleTranslation::StackFrame and related classes and methods in MLIR to LLVM IR translation seem to define an applicable pattern we could use here as well. Not sure if there's a good way of refactoring and reusing these same classes both here and in MLIR, but my feeling is that the worst-case scenario of mirroring that approach in Flang lowering wouldn't introduce so much code duplication that it could be an issue.

What do you think about that alternative?

[tblah]

Okay if you are happy for HostEvalInfo to become externally visible, I will add that to this PR. I didn't put these into AbstractConverter to begin with because I wasn't sure if it was appropriate to promote it outside of this file.

I think something like ModuleTranslation::StackFrame could work here but that is almost orthogonal. We need some sort of argument passed everywhere (equivalent to ModuleTranslation) which can provide access to the stack. Whether the stack is implemented by a few separate SmallVectors (as is done in this PR) or in a single vector holding a CRTP class (like ModuleTranslation::StackFrame) is easy to do once some sort of OpenMP lowering context is available.

What would you think about sharing all of the context for these calls in one big class (probably kept internal to this translation unit). For example,

// Instead of having loads of these
static mlir::omp::SomethingOp genSomethingOp(lower::AbstractConverter &converter, semantics::SemanticsContext &semaCtx, ...);

// Wrap it all in a class to store the common parameters
class OmpLowering {
public:
  ...
  void genOMP(const parser::OpenMPSomethingConstruct &construct);
  
private:
  mlir::omp::SomethingOp
  genSomethingOp(lower::pft::Evaluation &eval, mlir::Location loc, const ConstructQueue &queue, ConstructQueue::const_iterator item);
  
  lower::AbstractConverter &converter;
  semantics::SemanticsContext &semaCtx;
  ...
};

My hope here is to make it a smaller change next time the interface to all of these functions needs to change. I guess a StackFrame could go in here.

[ergawy]

Thank you Tom for taking a stab at this issue. In my opinion, we should probably take this opportunity to address it for both hostEvalInfo and sectionsStack in a slightly more generic way.

I think you're referring to this yourself in the "FIXME" comment you added for hostEvalInfo, but in my opinion it would be the AbstractConverter class the one that should hold a stack of generic information and provide basic facilities to introduce, query and remove objects from it. That way, we can produce information on parent constructs that can then be queried by their children, which would address the current need for any global variables here.

The ModuleTranslation::StackFrame and related classes and methods in MLIR to LLVM IR translation seem to define an applicable pattern we could use here as well. Not sure if there's a good way of refactoring and reusing these same classes both here and in MLIR, but my feeling is that the worst-case scenario of mirroring that approach in Flang lowering wouldn't introduce so much code duplication that it could be an issue.

What do you think about that alternative?

I think that's a very good idea. Both ModuleTranslation and AbstractConverter serve a similar purpose when it comes to stacking nested constructs/ops.

[tblah]

I think that's a very good idea. Both ModuleTranslation and AbstractConverter serve a similar purpose when it comes to stacking nested constructs/ops.

Oh you want to put the stack in AbstractConverter. Yes that could avoid most of the churn in this PR. LGTM. Do you want me to do it or are you already working on it?

[ergawy]

I think that's a very good idea. Both ModuleTranslation and AbstractConverter serve a similar purpose when it comes to stacking nested constructs/ops.

Oh you want to put the stack in AbstractConverter. Yes that could avoid most of the churn in this PR. LGTM.

That's what I got from Segio's suggestion (correct me if I am wrong @skatrak). So we would have something like StackFrame and StackFrameBase exposed by the AbstractConverter and then specialize/inherit the different kinds of frames needed for OpenMP inside OpenMP.cpp.

Do you want me to do it or are you already working on it?

Not working on that atm.

[skatrak]

Oh you want to put the stack in AbstractConverter. Yes that could avoid most of the churn in this PR. LGTM.

That's what I got from Segio's suggestion (correct me if I am wrong @skatrak). So we would have something like StackFrame and StackFrameBase exposed by the AbstractConverter and then specialize/inherit the different kinds of frames needed for OpenMP inside OpenMP.cpp.

Yes, that's what I meant to say. We would have an llvm::SmallVector<StackFrameBase> inside of AbstractConverter and the concrete definitions to hold HostEvalInfo or const parser::OpenMPSectionsConstruct * would be local to OpenMP.cpp, so we wouldn't have to make HostEvalInfo externally visible. I think that is the main advantage of that pattern, it's flexible enough to let us store whatever we need without having to know about data types introduced by other parts of lowering.

What would you think about sharing all of the context for these calls in one big class (probably kept internal to this translation unit).

That would be a separate patch in my opinion, but I think it would certainly be quite helpful regardless of how we deal with the context, since we're sharing quite a big chunk of state between many helper functions through arguments.