[flang] Use BIND name, if any, when consolidating common blocks #65613
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Semantics is maintaining a list of common blocks appearing in the file to allow for the extension where a common may appear with different size. It needs to account for BIND(C) name when matching the names. What matters is to merge the common block with similar global name. This patch allows merging BIND(C) and non BIND(C) common blocks that end up having the same name in the object file (previously, this crashed when lowering FIR to LLVM). This is not well specified, but matches what gfortran, ifort and nvfortran are doing and what would happen if the common block had been in different files. To support this use case, the mangling of common blocks (_QC) has to be dropped for all common blocks. There was no such mangling for BIND(C) common already anyway, and the mangling did not bring a lot of valuable info since there was no scope information (common block are common to all scopes). If needed, an MLIR attribute could be added instead to flag that a fir.global is a common block (the linkage is a good hint already, although there may be use case for using this linkage for objects that are not user defined common block form the Fortran source).
klausler
approved these changes
Sep 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch changes how common blocks are aggregated and named in lowering in order to:
fix one obvious issue where BIND(C) and non BIND(C) with the same Fortran name were "merged"
go further and deal with a derivative where the BIND(C) C name matches the assembly name of a Fortran common block. This is a bit unspecified IMHO, but gfortran, ifort, and nvfortran "merge" the common block without complaints as a linker would have done. This required getting rid of all the common block mangling early in FIR (_QC) instead of leaving that to the phase that emits LLVM from FIR because BIND(C) common blocks did not have mangled names. Care has to be taken to deal with the underscoring option of flang-new.
See added flang/test/Lower/HLFIR/common-block-bindc-conflicts.f90 for an illustration.