Page MenuHomePhabricator

[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

Repository
rL LLVM

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
3547 ↗(On Diff #40762)

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
3548 ↗(On Diff #40915)

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.

3549 ↗(On Diff #40915)

if (Align > OverflowAreaAlign)

3559 ↗(On Diff #40915)

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);
3562 ↗(On Diff #40915)

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
241 ↗(On Diff #41142)

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.

3568 ↗(On Diff #41142)

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 ↗(On Diff #41297)

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

216 ↗(On Diff #41297)

LLVM code style is to always leave the braces there.

3087 ↗(On Diff #41297)

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
216 ↗(On Diff #41498)

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.