This is an archive of the discontinued LLVM Phabricator instance.

Make global aliases have symbol size equal to their type
ClosedPublic

Authored by john.brawn on Jun 30 2015, 7:22 AM.

Details

Summary

This is mainly for the benefit of GlobalMerge, so that an alias into a MergedGlobals variable has the same size as the original non-merged variable.

Diff Detail

Repository
rL LLVM

Event Timeline

john.brawn updated this revision to Diff 28778.Jun 30 2015, 7:22 AM
john.brawn retitled this revision from to Make global aliases have symbol size equal to their type.
john.brawn updated this object.
john.brawn edited the test plan for this revision. (Show Details)
john.brawn set the repository for this revision to rL LLVM.
john.brawn added a subscriber: Unknown Object (MLST).
rafael edited edge metadata.Jun 30 2015, 7:25 AM

Why do you need to print the .size in the assembly? The assembler
itself (MC/gas) propagates the size.

Why do you need to print the .size in the assembly? The assembler
itself (MC/gas) propagates the size.

Yes, but it propagates it from the aliasee. In global-merge-2.ll, for example, if you generate object output then inspect the symbol table it says

0000000000000000 g .bss 0000000c x
0000000000000004 g .bss 0000000c y
0000000000000008 g .bss 0000000c z

which says that x is 12-bytes in size starting at offset 8 inside the .bss section. As x was originally (before GlobalMerge) a 4-byte variable this is slightly surprising, plus the .bss section is 12 bytes long so z apparently extends past the end of the .bss section.

which says that x is 12-bytes in size starting at offset 8 inside the .bss section. As x was originally (before GlobalMerge) a 4-byte variable this is slightly surprising, plus the .bss section is 12 bytes long so z apparently extends past the end of the .bss section.

That should have been 'z is 12-bytes in size starting at offset 8'.

I see,

Given

.size b, 8

b:

.quad 4
a = b + 4

The assembler says that "a" has size 8 :-(

I already think that the assembler is trying to do too much in here,
so I agree that printing a .size is the way to go.

const DataLayout *DL = TM.getDataLayout();

Please get the DataLayout from the Module.

David, would this be in your way for removing types from pointers? If not, LGTM.

Thanks,
Rafael

john.brawn updated this revision to Diff 29169.Jul 7 2015, 6:27 AM
john.brawn added a reviewer: dblaikie.

Adjusted to get the DataLayout from the Module.

dblaikie added inline comments.Jul 14 2015, 10:24 AM
lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1118

Yeah, this would be a problem for the typeless pointer work. If you can avoid introducing calls to PointerType::getElementType it'll make my migration work a bit easier.

Chances are, what we need, is GlobalValue to have a value type (I think I've added this to GlobalVariable or some other things in the GlobalValue hierarchy - if all of them need it, we can just add it up in GlobalValue) that can be retrieved directly, because their actual type will eventually just be an opaque pointer.

I don't know much about aliases - they can be differently typed than the thing they alias? Not sure what that means or what their size means either...

I don't know much about aliases - they can be differently typed than the thing they alias?

Yes.

Not sure what that means or what their size means either...

Pretty much whatever the "user" wants it to mean. In this case the
only real user we have is the concatenation of global variables, so
the desire is for the size to represent the original size.

Cheers,
Rafael

john.brawn added inline comments.Jul 15 2015, 5:03 AM
lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1118

It looks like you've added getValueType() to GlobalValue, which currently just returns getType()->getElementType() though I guess that will be changed sometime in the future. I'll use that instead.

john.brawn updated this revision to Diff 29775.Jul 15 2015, 5:19 AM

Use getValueType() instead getType()->getElementType().

rafael accepted this revision.Jul 16 2015, 2:48 PM
rafael edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jul 16 2015, 2:48 PM
This revision was automatically updated to reflect the committed changes.