[NFC] Decouple variable instantiation from the bridge

[jeanPerier]

This patch is a NFC towards lowering pointer globals. It also move 1300 lines out from bridge.cpp that are not strongly coupled with the rest of bridge.cpp code.

The patch is organized in two commits to make the all code shuffling easier to review:

• first commit modifies the code, making all variable instantiation related static functions that takes an AbstractConverter and a SymMap argument when relevant.
• second commit moves the code with absolutely no changes other than the added header, the cmake file, and a few Fortran::lower added to account for the interface.

The main goal of this patch is to allow InstantiateVar(pft::Variable) to be able to happen in a different SymMap than the current one in the bridge. The motivation for this is that when lowering a global initializer, all current mapping in the bridge are wrong, they point to SSA value from another region than the globalOp region. Which means:

1. all the symbols required by the initialization expression need to be instantiated in the GlobalOp region.
2. we really want to avoid using SSA from the current region by accident when lowering the globalOp because this leads to nasty no-obvious crashes in mlir.

To satisfy the two constraints, I studied three options:

• alternative 1: cloning the required instantiated symbols from the current region into the globalOp region. There is an mlir::Operation::clone methods, but though it mentions being a deep copy, it really does not copy the operand parentOp, this has to be done manually. So it is a bit hacky. I also studied cloning blocks, but it brought some local allocation and other illegal ops that CSE does not always eliminate and lead to crashes in llvm codegen. But the real stopping point is that when lowering a module, the variables are not instantiated, we only define the GloablOp of the variables owned by the module (that means we do not generate any address_of for variables since we are lowering the module, not a function using the variables). So if we wanted the clone method to work there, we would need to start instantiating all variables referred by a module, even if we will just never use these SSA values in 99% of the cases. It seems the price should only be paid when lowering the global initializer instead.

• alternative 2: pushing a new SymMap scope. It could be made to work, but would require to always force symbols in mapSymbolAttributes, which seems unsafe. I am reluctant to make that possible. Also, if one misses any dependencies, and some symbols are not shadowed, which is hard to guarantee could never happen on edge cases initial data target, we get awful crashes instead of a nice "Symbol is not mapped" error that we would get using a new map.

• alternative 3: make localSymbols member a pointer, that can be temporarily set to a new map. It would work, but would prevent the code split that I am proposing to make bridge.cpp more focus on statement/construct/functions lowering (we would need to expose the localSymbols member to AbstractConverter clients, and allow them to change it temporarily). I think it is also bringing new AbstractConverter state issue that we probably just want to keep away from.

So the alternative I am proposing is to specify the SymMap in which a pft::Variable must be instantiated and it seems to be the most robust solution to me. With this, I will be able to create a new symbolMap to safely instantiate the initial data target dependencies and the initial data target expression itself.

To avoid using the wrong map by mistake in the 1300 lines that are instantiating variable, it appeared a lot safer to me to move all the instantiation related code outside of the bridge (whose member function can easily silently plug into the wrong map). I think it also makes a pretty neat split of a feature that can live by itself.

[schweitzpgi]

That's a lot of code to move.

You might want to move the ConvertVariable.h into the lib/Lower directory, to avoid exposing the SymMap type as something exported by lowering.

[jeanPerier]

You might want to move the ConvertVariable.h into the lib/Lower directory, to avoid exposing the SymMap type as something exported by lowering.

Good point, I moved the header.