Fix PR#24142

[Carrot]

Summary

Function getSPAdjust doesn't consider the stack alignment, its usage in PEI::replaceFrameIndices causes wrong offset is used for stack objects.

This patch adds a function to align the SP adjustment to the correct alignment, and call it from getSPAdjust and ARM backend. It can/should also be used by other backends if SP adjustment is need to be aligned.

[echristo]

Test?

[chandlerc]

In addition to needing a test, Reid should probably have a look as he's thought a lot about stack alignment and adjustment.

[Carrot]

A testcase is added, before this patch, wrong address [sp, #2116] is generated.

[majnemer]

Can you use RoundUpToAlignment?

[Carrot]

Use RoundUpToAlignment to calculate the aligned SP adjustment.

[Carrot]

ping

[hfinkel]

include/llvm/Target/TargetFrameLowering.h

79

Can't you just write this as:

SPAdj = -RoundUpToAlignment(-SPAdj, StackAlignment);

test/CodeGen/ARM/align-sp-adjustment.ll

4

How stable is this number?

[Carrot]

test/CodeGen/ARM/align-sp-adjustment.ll

5

This number is the sum of stack frame and the space of outward parameter(8). The stack frame is:

push.w  {r4, r5, r6, r7, r8, r9, r10, r11, lr}

.Ltmp0:

.cfi_def_cfa_offset 36

.Ltmp1:

.cfi_offset lr, -4

.Ltmp2:

.cfi_offset r11, -8

.Ltmp3:

.cfi_offset r10, -12

.Ltmp4:

.cfi_offset r9, -16

.Ltmp5:

.cfi_offset r8, -20

.Ltmp6:

.cfi_offset r7, -24

.Ltmp7:

.cfi_offset r6, -28

.Ltmp8:

.cfi_offset r5, -32

.Ltmp9:

.cfi_offset r4, -36 
.pad    #2064
sub.w   sp, sp, #2064
.pad    #12
sub     sp, #12

12 is padding, 2064 is the size of local variable, the rest are for callee saved registers. The only possible changing size is the space for callee saved registers, and the only possible optimization that can impact it is register allocation. There are several function call in the loop, most of the callee saved registers are used to keep variables cross function call, so this looks unlikely to change.

[Carrot]

ping

[rnk]

Michael added getSPAdjust recently, so he can comment on what it should be doing.

lib/CodeGen/TargetInstrInfo.cpp

664

I don't think this is the right place to fix this. getSPAdjust is also used on X86 to return SP adjustment values for PUSH, POP, CALL, and others. These typically produce values that are not a multiple of stack alignment (4 / 8 vs 16).

[rnk]

test/CodeGen/ARM/align-sp-adjustment.ll

5

I think you can reduce this test case further to make it more robust and easier to understand.

The string parameters are probably not needed, and can be i32's or i8* null values to make it shorter.

The array can instead be a simple array of i32, like %params = alloca i32, 516, which gives an i32*, which requires less GEP manipulation.

15

I'm surprised this assembles, I don't see a definition of #3 or any other attribute sets.

[mkuper]

lib/CodeGen/TargetInstrInfo.cpp

664

I think this actually is the right place for it.
This looks like a long-standing bug from before I factored this code out of PEI and into TFI.

From my commit message on the refactoring, emphasis is new:
"PEI tries to keep track of how much starting or ending a call sequence adjusts the stack pointer by, so that it can resolve frame-index references. Currently, it takes a very simplistic view of how SP adjustments are done - both FrameStartOpcode and FrameDestroyOpcode adjust it exactly by the amount written in its first argument.

This view is in fact incorrect for some targets (*e.g. due to stack re-alignment*, or because it may want to adjust the stack pointer in multiple steps). However, that doesn't cause breakage, because most targets (the only in-tree exception appears to be 32-bit ARM) rely on being able to simplify the call frame pseudo-instructions earlier, so this code is never hit. "

The newly-created (in a later commit) X86 version of getSPAdjust actually does take alignment into account, but, unfortunately, I never went back and fixed the ARM version - I didn't know it's broken in practice.

[rnk]

lib/CodeGen/TargetInstrInfo.cpp

664

Do you think the new behavior of aligning the outgoing call argument space should live in the ARM backend or the generic Target layer?

I'm surprised that the backends aren't responsible for building aligned call stack adjustments. I guess this ties into the whole crazy phase ordering of stack realignment, where we try to support late stack realignment if the register allocator wants it.

[mkuper]

lib/CodeGen/TargetInstrInfo.cpp

664

The generic target layer makes sense to me. The fact the call stack adjustment instructions don't take alignment into account isn't backend-specific.
I should probably also refactor the X86 code to call into the generic layer for the framesetup/framedestroy case. Will try to do that once this patch goes in.

And yes, I was surprised at that either.

[Carrot]

test/CodeGen/ARM/align-sp-adjustment.ll

5

I tried to change the string to integer 0, unfortunately it failed to reproduce the bug. The reason is the compiler need to generate some instructions to compute the address of string, store it in some register, and moved the code out of the loop since it is loop invariant. If it is changed to some constant, then it need not to be stored in register, the extra register can be used store the value of the fifth parameter, the load is moved out of the loop, so avoid the problem code sequence.

Similarly if I change the data type of %params, the address computation is also changed, and the register usage is changed, causes the fifth parameter loaded before the loop, and failed to reproduce the bug.

[rnk]

OK, seems reasonable.

[Eugene.Zelenko]

Committed in rL245253.

include/llvm/Target/TargetFrameLowering.h

   ///
   unsigned getStackAlignment() const { return StackAlignment; }
 
+  /// alignSPAdjust - This method aligns the stack adjustment to the correct
+  /// alignment.
+  ///
+  int alignSPAdjust(int SPAdj) const {
+    if (SPAdj < 0) {
+      SPAdj = -SPAdj;
+      SPAdj = (SPAdj + StackAlignment - 1) / StackAlignment * StackAlignment;
+      SPAdj = -SPAdj;
+    } else {
+      SPAdj = (SPAdj + StackAlignment - 1) / StackAlignment * StackAlignment;
+    }
+    return SPAdj;
+  }
+
   /// getTransientStackAlignment - This method returns the number of bytes to
   /// which the stack pointer must be aligned at all times, even between
   /// calls.

lib/CodeGen/TargetInstrInfo.cpp

     return 0;
  
   int SPAdj = MI->getOperand(0).getImm();
+  SPAdj = TFI->alignSPAdjust(SPAdj);
 
   if ((!StackGrowsDown && MI->getOpcode() == FrameSetupOpcode) ||
        (StackGrowsDown && MI->getOpcode() == FrameDestroyOpcode))

lib/Target/ARM/ARMFrameLowering.cpp

       // We need to keep the stack aligned properly.  To do this, we round the
       // amount of space needed for the outgoing arguments up to the next
       // alignment boundary.
-      unsigned Align = getStackAlignment();
-      Amount = (Amount+Align-1)/Align*Align;
+      Amount = alignSPAdjust(Amount);
 
       ARMFunctionInfo *AFI = MF.getInfo<ARMFunctionInfo>();
       assert(!AFI->isThumb1OnlyFunction() &&

test/CodeGen/ARM/align-sp-adjustment.ll

+; RUN: llc -mtriple=thumbv7 -o - %s | FileCheck %s
+
+; CHECK: [sp, #2120]
+
+%struct.struct_2 = type { [172 x %struct.struct_1] }
+%struct.struct_1 = type { i32, i32, i32 }
+
+@.str = private unnamed_addr constant [2 x i8] c"a\00", align 1
+@.str.1 = private unnamed_addr constant [2 x i8] c"b\00", align 1
+@.str.2 = private unnamed_addr constant [2 x i8] c"c\00", align 1
+@.str.3 = private unnamed_addr constant [2 x i8] c"d\00", align 1
+
+declare i32* @_Z4bar3iiPKcS0_i(i32, i32, i8*, i8*, i32) #3
+declare void @_Z4bar1i8struct_2(i32, %struct.struct_2* byval align 4) #3
+declare i32 @_Z4bar2PiPKc(i32*, i8*) #3
+
+define void @_Z3fooiiiii(i32 %p1, i32 %p2, i32 %p3, i32 %p4, i32 %p5) #0 {
+entry:
+  %params = alloca %struct.struct_2, align 4
+  %0 = bitcast %struct.struct_2* %params to i8*
+  br label %for.body
+
+for.body:
+  %i.015 = phi i32 [ 0, %entry ], [ %inc, %for.inc ]
+  %call = tail call i32* @_Z4bar3iiPKcS0_i(i32 %p1, i32 %p5, i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.str.1, i32 0, i32 0), i32 %i.015) #4
+  %cmp1 = icmp eq i32* %call, null
+  br i1 %cmp1, label %cleanup.8, label %for.inc
+
+for.inc:
+  %call2 = tail call i32 @_Z4bar2PiPKc(i32* %call, i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.str.2, i32 0, i32 0)) #4
+  %f1 = getelementptr inbounds %struct.struct_2, %struct.struct_2* %params, i32 0, i32 0, i32 %i.015, i32 0
+  store i32 %call2, i32* %f1, align 4
+  %call3 = tail call i32 @_Z4bar2PiPKc(i32* %call, i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.str.3, i32 0, i32 0)) #4
+  %f2 = getelementptr inbounds %struct.struct_2, %struct.struct_2* %params, i32 0, i32 0, i32 %i.015, i32 1
+  store i32 %call3, i32* %f2, align 4
+  %inc = add nuw nsw i32 %i.015, 1
+  %cmp = icmp slt i32 %inc, 4
+  br i1 %cmp, label %for.body, label %for.end
+
+for.end:
+  call void @_Z4bar1i8struct_2(i32 %p4, %struct.struct_2* byval nonnull align 4 %params) #4
+  br label %cleanup.8
+
+cleanup.8:
+  ret void
+}