This is an archive of the discontinued LLVM Phabricator instance.

[Power PC] fix calculating address of arguments on stack for variadic functions
ClosedPublic

Authored by spetrovic on Nov 20 2015, 5:22 AM.

Details

Summary

This patch fixes calculating address of arguments larger than 32 bit on stack for variadic functions (rounding up address to alignment) on ppc32 architecture.

Diff Detail

Event Timeline

spetrovic updated this revision to Diff 40762.Nov 20 2015, 5:22 AM
spetrovic retitled this revision from to [Power PC] fix calculating address of arguments on stack for variadic functions.
spetrovic updated this object.
spetrovic added reviewers: petarj, hfinkel, rjmccall.
spetrovic added a subscriber: cfe-commits.
hfinkel added inline comments.Nov 21 2015, 7:14 AM
lib/CodeGen/TargetInfo.cpp
3548

You need to use variable names consistent with our coding convention (OverflowArgArea, etc.).

spetrovic updated this revision to Diff 40915.Nov 23 2015, 6:36 AM
spetrovic marked an inline comment as done.
rjmccall added inline comments.Nov 23 2015, 4:34 PM
lib/CodeGen/TargetInfo.cpp
3549

This patch applies to more types than just double. You should probably add tests for some of the other cases, but if not, please at least make your commit message convey the intended generality.

3550

if (Align > OverflowAreaAlign)

3560

Would you mind extracting a function to do this realignment? There are 3-4 other places in this file that do exactly this. Using the existing code pattern from emitVoidPtrDirectVAArg — i.e. using an add instead of a non-inbounds GEP — seems reasonable.

I think this is a sensible prototype:

static Address emitRoundPointerUpToAlignment(CodeGenFunction &CGF, llvm::Value *Ptr, CharUnits Align, const llvm::Twine &Name);
3563

This code will be more readable if you keep Align as a CharUnits.

spetrovic updated this revision to Diff 41142.Nov 25 2015, 7:43 AM
spetrovic updated this object.
spetrovic marked 4 inline comments as done.
rjmccall added inline comments.Nov 25 2015, 2:45 PM
lib/CodeGen/TargetInfo.cpp
251

Thank you for extracting this.

First, this function deserves a doc comment now; I would suggest:

/// Dynamically round a pointer up to a multiple of the given alignment.

Also, this is now generically useful, so (1) please rename OverflowArgArea to something more generic, like Ptr, and (2) please use LLVM value names based on the name already in Ptr. That is, instead of "overflow_arg_area.align", please use

Ptr->getName () + ".aligned"

Finally, please look for the other places in this file that you could change to use this new function. There's one in emitVoidPtrDirectVAArg, and there are several other in other targets.

3552

Please sink OverflowArgArea into the if block, since you don't need it outside.

spetrovic updated this revision to Diff 41297.Nov 27 2015, 5:35 AM
spetrovic marked 2 inline comments as done.

Comments addressed.

rjmccall edited edge metadata.Nov 30 2015, 1:56 PM

Thank you. A few more style complaints; with those fixed, LGTM.

lib/CodeGen/TargetInfo.cpp
166

Minor code-style nitpick: please put the * next to the function name, not the type.

221

LLVM code style is to always leave the braces there.

3092

Braces.

spetrovic updated this revision to Diff 41498.Dec 1 2015, 6:23 AM
spetrovic edited edge metadata.
spetrovic marked 3 inline comments as done.

Comments addressed.

Please feel free to commit after you make this last change.

lib/CodeGen/TargetInfo.cpp
219

LLVM code style is to put the else on the same line as the closing brace.

spetrovic updated this revision to Diff 41613.Dec 2 2015, 6:00 AM
spetrovic marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.