Page MenuHomePhabricator

Place undefined globals in .bss instead of .data
ClosedPublic

Authored by varkor on Jan 3 2018, 10:27 AM.

Details

Summary

Following up on the discussion from http://lists.llvm.org/pipermail/llvm-dev/2017-April/112305.html, undef values are now placed in the .bss as well as null values. This prevents undef global values taking up potentially huge amounts of space in the .data section.

The following two lines now both generate equivalent .bss data:

@vals1 = internal unnamed_addr global [20000000 x i32] zeroinitializer, align 4
@vals2 = internal unnamed_addr global [20000000 x i32] undef, align 4 ; previously unaccounted for

This is primarily motivated by the corresponding issue in the Rust compiler.

Diff Detail

Repository
rL LLVM

Event Timeline

varkor created this revision.Jan 3 2018, 10:27 AM
varkor edited the summary of this revision. (Show Details)

Please post diffs with context (see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

Needs a regression test (probably in test/CodeGen/X86), to show that we are in fact generating the expected code.

Does this do the right thing for constant structs which have some null and some undef members?

varkor updated this revision to Diff 128552.Jan 3 2018, 1:05 PM
  • Added support for structs with a combination of null and undef fields.
  • Added a regression test.
efriedma added inline comments.Jan 3 2018, 1:38 PM
lib/Target/TargetLoweringObjectFile.cpp
66 ↗(On Diff #128552)

What if E is a ConstantAggregate?

varkor updated this revision to Diff 128576.Jan 3 2018, 3:22 PM

Added support for nested constant aggregates.

varkor marked an inline comment as done.Jan 7 2018, 10:13 AM

@efriedma: are there any other changes you can see that need to be made here?

efriedma added inline comments.Jan 17 2018, 2:10 PM
lib/Target/TargetLoweringObjectFile.cpp
63 ↗(On Diff #128576)

Maybe "for (auto Operand : C->operand_values())"?

varkor updated this revision to Diff 130300.EditedJan 17 2018, 3:35 PM

Use range-based for loop.

efriedma accepted this revision.Jan 17 2018, 3:36 PM

LGTM. Do you have commit access?

This revision is now accepted and ready to land.Jan 17 2018, 3:36 PM
varkor marked an inline comment as done.EditedJan 17 2018, 3:37 PM

Done. That is slightly neater!

No, I don't have commit access — if you could do that for me, that'd be great! Thank you!

efriedma requested changes to this revision.Jan 26 2018, 12:07 PM

I was going to apply this, but then I found there's a "make check" failure caused by this change:

[...]/llvm/test/CodeGen/ARM/memfunc.ll:425:10: error: expected string not found in input
; CHECK: arr8:
         ^
<stdin>:341:1: note: scanning from here
_arr9:
^
<stdin>:341:2: note: possible intended match here
_arr9:
 ^

Probably the test just needs to be updated, but please take a look.

This revision now requires changes to proceed.Jan 26 2018, 12:07 PM
varkor updated this revision to Diff 131707.Jan 28 2018, 8:28 AM

Fixed an ARM CodeGen test whose behaviour was changed by the patch. (The data for arr8 is now placed in the BSS, so the simplest fix seemed just to be no longer checking for p2align, which is no longer present.)

varkor updated this revision to Diff 132388.Feb 1 2018, 6:53 AM

As Rafael pointed out, we should still be checking the alignment of bss data — it's just no longer specified with p2align. I've updated the test to check for the alignment of arr8. I think this should now be ready to take a look at again!

efriedma accepted this revision.Feb 6 2018, 2:41 PM

LGTM.

How should I credit you in the commit message?

This revision is now accepted and ready to land.Feb 6 2018, 2:41 PM
varkor added a comment.EditedFeb 6 2018, 3:12 PM

Great, thanks! Just as "varkor" is great.

This revision was automatically updated to reflect the committed changes.