agozillon wrote: > > In Fortran, this patch breaks declare target on common blocks > > > Nekbone is also an example test which exhibits this failure: > > https://github.com/AMDComputeLibraries/Nekbone > > https://github.com/ROCm/aomp/blob/aomp-dev/bin/run_nekbone.sh > > @dpalermo Did you add a compile and executable test for this? References to > AMD repos are not that helpful as we continue development here. >
While true, it's fairly common for people to block or revert things if they're proven to break pre-existing code in their own buildbots and offer a common ground fix which we have so far. However, we could add the small fortran test Dan linked above to check-offload if you would prefer something in line inside the LLVM infrastructure as this is a working case in LLVM that this breaks not specific to downstream, it's just a test case that I hadn't bothered to add upstream yet (as I didn't think it was divergent enough from the similar cases, turns out I was wrong). The cross module variant that Nekbones shows is perhaps a bit more difficult, but entirely do-able I'd imagine. > Now looking at the merits of "common linkage for common blocks", it seems the > zero initialization already rules out that this is the right choice. Assuming > https://stackoverflow.com/a/49618639 is correct. > > Thus, Flang is wrong to use common linkage. I do not know what linkage flang > should use, but someone working on Flang should, I hope. I'll revisit this > next week assuming that @dpalermo or @agozillon have a patch for Flang so > this can be applied again. Whilst I agree that generally Fortran does indeed not initialize to zero by default, and Flang follows this currently for most types, it does not do this for globals it will zero initialize them (perhaps to help with cases like the above) unless I imagine prohibited by the specification (which I don't have handy as it's paywalled unfortunately), and the original author of the initializing (for the general global case, not the common block case specifically) has stated whilst not specification mandated it's fairly common behaviour: https://github.com/llvm/llvm-project/blob/main/flang/lib/Lower/ConvertVariable.cpp#L630, common blocks get a similar treatment in declareCommonBlock in the same file. But the above is more historical and digresses from your point which is mainly that you just want Flang to use weak linkage instead of common, whilst I'm happy to make the patch that will change common linkage to weak linkage, I'm not really wanting to be the one arguing for the change to this linkage if it is contentious as I don't understand the initial reasoning fully or know the fallout that would come from it, @jeanPerier may know why we use common linkage over weak however or may point you in the right direction of someone who does know the original reasoning. I won't be available after mid next week until the beginning of July. There is however, a bi-weekly meeting that would be great to attend for discussion if you believe the handling is wrong and wanted to change the Flang handling of this type. As an aside, there is an argument that in either case whether Flang is incorrect or otherwise, you probably do not want common linkage to echo onto the entry globals as by definition they are not compatible, you could of course shrug that off and say it's a user error which is the other side of the coin. https://github.com/llvm/llvm-project/pull/200964 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
