Page MenuHomePhabricator

Merge of global constants does not happen when constants have common linkage
Needs ReviewPublic

Authored by ramred01 on Mar 7 2019, 1:56 AM.

Details

Summary

Merge of global constants with common linkage does not happen even when the constants are used in successive instructions. It generates two separate labels for the two constants and hence materializes the two label addresses separately before loading the two constants, when merging the two constants and having a common label would have meant loading the label address once in a base register and then loading the constants using an offset from that label.

The problem here is that when the constants neither have external linkage nor internal linkage but have a common linkage, they are being skipped from the Global Merge analysis. This is causing such constants to be emitted separately with separate labels and the generated code materializes those multiple labels separately instead of materializing one label and then accessing the other constants with an offset from that label.

This fix checks for the values to have common linkage in addition to the external or internal linkages.

Diff Detail

Event Timeline

ramred01 created this revision.Mar 7 2019, 1:56 AM
dexonsmith resigned from this revision.Mar 7 2019, 10:25 AM
dexonsmith added a reviewer: ab.

+ab, do you still know this code?

The title and summary of this patch describe a problem, but they don't describe how this patch fixes that problem (which makes it hard to figure out if this patch is correctly fixing the problem). It should be more like "[GlobalMerge] Do XYZ: We have problem ABC, doing XYZ in GlobalMerge fixes it because ...". Also this patch needs tests.

ramred01 updated this revision to Diff 192597.Mar 28 2019, 3:41 AM
ramred01 retitled this revision from Merge of global constants not happenning to Merge of global constants does not happen when constants have common linkage.
ramred01 edited the summary of this revision. (Show Details)
ramred01 added a subscriber: llvm-commits.

Updated the summary.

Added a test case.

efriedma requested changes to this revision.Mar 28 2019, 11:19 AM
efriedma added a subscriber: efriedma.

This isn't correct; GlobalMerge is not legal for symbols with weak linkage, and there isn't any way to correctly emit an alias with "common" linkage in ELF.

(If you want to avoid common linkage for C code, you can build with -fno-common; this is probably a good idea for codebases which are compatible with it.)

This revision now requires changes to proceed.Mar 28 2019, 11:19 AM
ramred01 updated this revision to Diff 199265.May 13 2019, 7:33 AM

Updated the patch with a fix that does not try to merge globals with common linkage as the earlier patch did.

The call to createGlobalMergePass was not passing the mergeExternal parameter which should be set to True for all non-Mach-O ports. This was causing the merge of global constants to fail even when -fno-common was being passed.

Could you post this as a new patch? This seems very different from the original description/discussion.