This is an archive of the discontinued LLVM Phabricator instance.

Use Store Size not Alloc Size when Coercing
ClosedPublic

Authored by tjablin on Aug 27 2014, 6:38 PM.

Details

Reviewers
jmolloy
Summary

Previously, EnterStructPointerForCoercedAccess used Alloc size when determining how to convert. This was problematic, because there were situations were the alloc size was larger than the store size. For example, if the first element of a structure were i24 and the destination type were i32, the old code would generate a GEP and a load i24. The code should compare store sizes to ensure the whole object is loaded. I have attached a test case.

This patch modifies the output of arm64-be-bitfield.c test case, but the new IR seems to be equivalent, and after -O3, the compiler generates identical ARM assembly. (asr x0, x0, #54) All tests pass with r216480. Thanks!

Diff Detail

Event Timeline

tjablin updated this revision to Diff 13016.Aug 27 2014, 6:38 PM
tjablin retitled this revision from to Use Store Size not Alloc Size when Coercing.
tjablin updated this object.
tjablin edited the test plan for this revision. (Show Details)
tjablin added a reviewer: jmolloy.
tjablin added a subscriber: Unknown Object (MLST).
jmolloy requested changes to this revision.Aug 28 2014, 2:01 AM
jmolloy edited edge metadata.

Hi Thomas,

This generally looks good to me with one change.

Cheers,

James

lib/CodeGen/CGCall.cpp
647–649

This comment is confusing. "Use the store size and not the alloca size here to ensure we will actually load the whole object" - But the alloca size is always greater than or equal to the store size. So the comment seems wrong - if we use the alloca size, we are also guaranteed to load the whole object.

Also please terminate sentences with a full-stop (.).

This revision now requires changes to proceed.Aug 28 2014, 2:01 AM
tjablin updated this revision to Diff 13037.Aug 28 2014, 7:59 AM
tjablin edited edge metadata.

I have adjusted the comment as per your request. If you are satisfied, could you please push it upstream for me. Thanks.

tjablin updated this revision to Diff 13063.Aug 28 2014, 4:42 PM
tjablin edited edge metadata.

Add REQUIRES: aarch64-registered-target since the new test looks at the ARM assembly.

jmolloy accepted this revision.Sep 2 2014, 2:42 AM
jmolloy edited edge metadata.

Review closed - code was committed.

This revision is now accepted and ready to land.Sep 2 2014, 2:42 AM
jmolloy closed this revision.Sep 2 2014, 2:42 AM