[PATCH] [ARM] Align stack objects passed to functions

[hfinkel at anl.gov]

REPOSITORY
  rL LLVM

================
Comment at: include/llvm/Target/TargetLowering.h:980
@@ +979,3 @@
+  /// Return true if the alloca arguments to CI should be aligned. If so then
+  /// AllocaSize is set to the minimum size the allocated object must be to be
+  /// aligned and AllocaAlign is set to the alignment the alloca is to be given.
----------------
Please name AllocaSize -> MinAllocaSize to make its purpose clearer.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:1235
@@ +1234,3 @@
+  // idea
+  unsigned AllocaSize = 0, AllocaAlign = 0;
+  if (TLI && TD && TLI->shouldAlignAllocaArgs(CI, AllocaSize, AllocaAlign)) {
----------------
Don't initialize these here; that makes it clear that they must be set by the TLI call, and allows mistakes to be caught by tools that detect use of undefined values (valgrind, etc.).

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:1236
@@ +1235,3 @@
+  unsigned AllocaSize = 0, AllocaAlign = 0;
+  if (TLI && TD && TLI->shouldAlignAllocaArgs(CI, AllocaSize, AllocaAlign)) {
+    assert(AllocaAlign != 0 && "shouldAlignAllocaArgs must set AllocaAlign");
----------------
john.brawn wrote:
> rengolin wrote:
> > john.brawn wrote:
> > > rengolin wrote:
> > > > I'm a bit averse of setting parameters for the computation inside a conditional and having to assert right after every call. I'd rather have (or reuse) some default size/alignment parameters from elsewhere.
> > > If an implementation of shouldAlignAllocaArgs were to not set AllocaAlign then that indicates that it's not doing what it should, so failing an assert is better than hiding that by silently using a default value.
> > I'm not against the assert, I'm against passing a reference for AllocaSize and AllocaAlign on a check call. The call is not explicit that it's setting the two values here, nor is it on TargetLowering's default implementation.
> > 
> > A more clear way would be to make a function whose only job is to set those variables and make it explicit on its name, like "setAlignAllocaArgs(CI, Size, Align)" and add "Size > 0" to the conditional. You can avoid the assert in that case, too.
> > 
> > Also, your check for TLI is redundant, since you already check for TD.
> Doing it this way (return a bool, set a reference/pointer argument only if returning true) seems to be the standard way these queries are done in TargetInfo - see allowsMisalignedMemoryAccesses, getStackCookieLocation, GetAddrModeArguments.
> I'm not against the assert, I'm against passing a reference for AllocaSize and AllocaAlign on a check call. The call is not explicit that it's setting the two values here, nor is it on TargetLowering's default implementation.

> A more clear way would be to make a function whose only job is to set those variables and make it explicit on its name, like "setAlignAllocaArgs(CI, Size, Align)" and add "Size > 0" to the conditional. You can avoid the assert in that case, too.

I disagree. So long as the values are not initialized above the call, it is clear that the call must be setting them (and returning true if it has done so). This pattern is used by a number of other callbacks, even some in this file (getStackCookieLocation, getPreIndexedAddressParts, getPostIndexedAddressParts, ShrinkDemandedConstant, etc.), and I think  it is appropriate here.

http://reviews.llvm.org/D7908

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/