This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Align global variables passed to memory intrinsics
ClosedPublic

Authored by john.brawn on Mar 18 2015, 11:06 AM.

Details

Summary

Fill in the TODO in CodeGenPrepare::OptimizeCallInst so that global variables that are passed to memory intrinsics are aligned in the same way that allocas are.

Diff Detail

Repository
rL LLVM

Event Timeline

john.brawn retitled this revision from to [ARM] Align global variables passed to memory intrinsics.
john.brawn updated this object.
john.brawn edited the test plan for this revision. (Show Details)
john.brawn added a reviewer: hfinkel.
john.brawn set the repository for this revision to rL LLVM.
john.brawn added a subscriber: Unknown Object (MLST).
hfinkel added inline comments.Mar 20 2015, 10:28 AM
lib/CodeGen/CodeGenPrepare.cpp
1249 ↗(On Diff #22198)

If you add a continue here, then you might end up skipping some logic further down that would otherwise apply, no?

1257 ↗(On Diff #22198)

Why? Do the alignments need to match?

1260 ↗(On Diff #22198)

Do you want this or hasUniqueInitializer() or hasDefinitiveInitializer()?

john.brawn added inline comments.Mar 24 2015, 7:45 AM
lib/CodeGen/CodeGenPrepare.cpp
1249 ↗(On Diff #22198)

That's what we want: this is the 'offset must be a multiple of the alignment' check, and if it's not true we don't want to align for both allocas and global variables.

1257 ↗(On Diff #22198)

http://llvm.org/docs/LangRef.html#global-variables says:
Targets and optimizers are not allowed to over-align the global if the global has an assigned section. In this case, the extra alignment could be observable: for example, code could assume that the globals are densely packed in their section and try to iterate over them as an array, alignment padding would break this iteration.

1260 ↗(On Diff #22198)

I think that hasUniqueInitializer is what we want. We want to make sure that increasing the alignment of this GlobalVariable will definitely increase the alignment in the final executable, i.e. we won't get a definition of this variable from another object that doesn't have this increased alignment.

john.brawn updated this revision to Diff 22575.Mar 24 2015, 8:03 AM

Change to use hasUniqueInitializer instead of hasInitializer.

rengolin added inline comments.Mar 27 2015, 2:40 AM
lib/CodeGen/CodeGenPrepare.cpp
1249 ↗(On Diff #22198)

I agree, this is a continue on the local block, only, and it has the same semantics as previously.

1261 ↗(On Diff #22198)

Why is this necessary?

john.brawn added inline comments.Mar 27 2015, 3:30 AM
lib/CodeGen/CodeGenPrepare.cpp
1261 ↗(On Diff #22198)

As I said to Hal previously, http://llvm.org/docs/LangRef.html#global-variables says:
Targets and optimizers are not allowed to over-align the global if the global has an assigned section. In this case, the extra alignment could be observable: for example, code could assume that the globals are densely packed in their section and try to iterate over them as an array, alignment padding would break this iteration.

rengolin accepted this revision.Mar 27 2015, 3:36 AM
rengolin added a reviewer: rengolin.

LGTM. But please, let Hal reply to your questions before commit.

lib/CodeGen/CodeGenPrepare.cpp
1261 ↗(On Diff #22198)

Sorry, I missed the "section" part when I read that. :)

This revision is now accepted and ready to land.Mar 27 2015, 3:36 AM

Hal, any comments on my comments on your comments? If not I'll check this in.

Hal: pinging you to check if this is OK to commit.

hfinkel accepted this revision.Apr 11 2015, 7:03 PM
hfinkel edited edge metadata.

Hal: pinging you to check if this is OK to commit.

LGTM too, thanks!

This revision was automatically updated to reflect the committed changes.