[PATCH] [ARM] Fix large stack alignment codegen bug for ARM and Thumb2 targets

[James Molloy]

Hi Kristof,

Thanks for working on this. Comments inline.

Cheers,

James


REPOSITORY
  rL LLVM

================
Comment at: lib/Target/ARM/ARMExpandPseudoInsts.cpp:890
@@ -889,2 +889,3 @@
           // Emit bic r6, r6, MaxAlign
+          // FIXME: what to do when MaxAlign doesn't fit in BIC's imm field?
           unsigned bicOpc = AFI->isThumbFunction() ?
----------------
I'd turn this FIXME into an assert. Then, you may end up with some buildbot providing you with a test case!

Either way, it's a silent fault so let's fail hard.

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:214
@@ -213,1 +213,3 @@
 
+// Emit an instruction sequence that will align the address in register Reg
+// by zero-ing out the lower bits.
----------------
Doc comments should use three slashes (///)

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:225
@@ +224,3 @@
+                                     DebugLoc DL, const unsigned Reg,
+                                     const unsigned Alignment,
+                                     const bool mustBeSingleInstruction) {
----------------
Might be worth mentioning in the doc comment whether Alignment is expected to be M or N in:

M = 1 << N.

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:226
@@ +225,3 @@
+                                     const unsigned Alignment,
+                                     const bool mustBeSingleInstruction) {
+  const ARMSubtarget &AST = MF.getTarget().getSubtarget<ARMSubtarget>();
----------------
Coding style: MustBeSingleInstruction.

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:228
@@ +227,3 @@
+  const ARMSubtarget &AST = MF.getTarget().getSubtarget<ARMSubtarget>();
+  const bool canUseBFC = AST.hasV6T2Ops() || AST.hasV7Ops();
+  const unsigned AlignMask = Alignment - 1;
----------------
CanUseBFC

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:231
@@ +230,3 @@
+  const unsigned nrBitsToZero = countTrailingZeros(Alignment);
+  assert(!AFI->isThumb1OnlyFunction());
+  if (!AFI->isThumbFunction()) {
----------------
... && "Thumb1 alignment not supported!") or something

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:252
@@ +251,3 @@
+    } else {
+      assert(!mustBeSingleInstruction);
+      AddDefaultCC(AddDefaultPred(
----------------
Where's the guarantee that we can't trigger this assertion in normal operation? If we can't use BFC, and AlignMask is > 255? Is this expected to happen?

If not, please add a "&& "Reason!"" to the assert.

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:1145
@@ -1092,4 +1144,3 @@
   unsigned MaxAlign = MF.getFrameInfo()->getMaxAlignment();
-  AddDefaultCC(AddDefaultPred(BuildMI(MBB, MI, DL, TII.get(Opc), ARM::R4)
-                              .addReg(ARM::R4, RegState::Kill)
-                              .addImm(MaxAlign - 1)));
+  // must set parameter mustBeSingleInstruction to true, since
+  // skipAlignedDPRCS2Spills expects exactly 3 instructions to perform
----------------
Please write comments in full sentences: "We must set the parameter..."

http://reviews.llvm.org/D6844

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