This is an archive of the discontinued LLVM Phabricator instance.

Fix AAPCS non-compliance caused by very large structs
ClosedPublic

Authored by olista01 on Jun 25 2014, 9:32 AM.

Details

Reviewers
rengolin
Summary

This is a fix to the code in clang which inserts padding arguments to ensure that the ARM backend can emit AAPCS-VFP compliant code. This code needs to track the number of registers which have been allocated in order to do this. When passing a very large struct (>64 bytes) by value, clang emits IR which takes a pointer to the struct, but the backend converts this back to passing the struct in registers and on the stack. The bug was that this was being considered by clang to only use one register, meaning that there were situations in which padding arguments were incorrectly emitted by clang.

Diff Detail

Event Timeline

olista01 updated this revision to Diff 10837.Jun 25 2014, 9:32 AM
olista01 retitled this revision from to Fix AAPCS non-compliance caused by very large structs.
olista01 updated this object.
olista01 edited the test plan for this revision. (Show Details)
olista01 added a subscriber: Unknown Object (MLST).
rengolin added inline comments.
lib/CodeGen/TargetInfo.cpp
4207

Couldn't this be:

NumRegs = (getContext().getTypeSize(Ty) + 63) / 32;
markAllocatedGPRs(2, NumRegs);

and avoid the extra multiplication?

Inline comment

lib/CodeGen/TargetInfo.cpp
4207

The division by 64 is part of rounding up to the nearest multiple of 64 [0], and this simplification would give different results if the size is, for example, 64 bits. That said, this code is only used when the size is greater than 64 bytes, meaning it will always use all available GPRs, so I only left the calculations in to make the intention clear. I wouldn't be averse to replacing both arms of the "if" with markAllocatedGPRs(1, 4) and a comment, if you would prefer that?

[0] This is because all types in the AAPCS have a size which is a multiple of their alignment.

rengolin added inline comments.Jun 27 2014, 6:02 AM
lib/CodeGen/TargetInfo.cpp
4207

Of course, silly me.

I think making it 4 and explaining why would make more sense.

olista01 updated this revision to Diff 10929.Jun 27 2014, 6:23 AM

Simplify, with comment explaining why this can be done.

This patch also fixes the (textual) alignment of the NumRequired parameter in the definition of ARMABIInfo::markAllocatedGPRs, which is present in the patch I uploaded but not showing up in Phab. This may be because it is a whitespace-only change?

rengolin accepted this revision.Jun 27 2014, 6:58 AM
rengolin added a reviewer: rengolin.

LGTM.

Thanks!
--renato

This revision is now accepted and ready to land.Jun 27 2014, 6:58 AM
olista01 closed this revision.Jun 27 2014, 7:07 AM

Thanks, Committed revision 211898.