[PATCH] ARM: add VLA extension for WoA Itanium ABI

[Tim Northover]

Hi Saleem,

A couple of minor comments, though it mostly looks reasonable.

Cheers.

Tim.

================
Comment at: docs/Extensions.rst:206
@@ +205,3 @@
+a dynamic stack allocation.  When emitting a variable stack allocation, a call
+to ``__chkstk`` is emitted unconditionally to ensure that guard pages are setup
+properly.  The emission of this stack probe emission is handled similar to the
----------------
I think the ABI for chkstk should be documented somewhere (possibly here and in code, possibly just in code). It looks like it takes and returns a value in r4, preserving all others (except for the inevitable lr).

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:7137
@@ +7136,3 @@
+  case CodeModel::Kernel:
+    BuildMI(*MBB, MI, DL, TII.get(ARM::tBL))
+      .addImm((unsigned)ARMCC::AL).addReg(0)
----------------
Tim Northover wrote:
> I don't think this is tracking R4 properly. This call is effectively an instruction that should have "R4<imp-use,kill>, R4<imp-def>". Otherwise different phases may assume R4 is unchanged in the interval.
> 
> Take a look at any normal call to an "i32 @foo(i32)" for a similar example with R0.
I think an assert or two in this function might be a good idea. At the very least, "isThumb" (instructions that look fine but have the wrong encoding are *really* nasty to track down). Possibly isWindows as well.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:7137-7140
@@ +7136,6 @@
+  case CodeModel::Kernel:
+    BuildMI(*MBB, MI, DL, TII.get(ARM::tBL))
+      .addImm((unsigned)ARMCC::AL).addReg(0)
+      .addExternalSymbol("__chkstk")
+      .addReg(ARM::R12, RegState::Implicit | RegState::Define | RegState::Dead);
+    break;
----------------
I don't think this is tracking R4 properly. This call is effectively an instruction that should have "R4<imp-use,kill>, R4<imp-def>". Otherwise different phases may assume R4 is unchanged in the interval.

Take a look at any normal call to an "i32 @foo(i32)" for a similar example with R0.

================
Comment at: lib/Target/ARM/ARMInstrInfo.td:5106
@@ +5105,3 @@
+                      [SDNPHasChain, SDNPSideEffect]>;
+let usesCustomInserter = 1, Uses = [R4] in
+  def __CHKSTK : PseudoInst<(outs), (ins), NoItinerary, [(__chkstk)]>;
----------------
Similarly, this should probably be marked "Defs = [R4]".

http://reviews.llvm.org/D4050