[PATCH] ARM and Thumb Segmented Stacks

[Tim Northover]

Hi Alex,

A very nice feature to have on ARM, I'm watching Rust with interest!

That said, I think the patch needs a little of work. Mostly annoying
formatting details, but I'm particularly concerned about two things:
+ The two TLS access methods
+ The separate ARM & Thumb implementations.

Cheers.

Tim.

+++ b/lib/Target/AArch64/AArch64Subtarget.h

There are no other AArch64 changes, this one probably shouldn't be there either.

+// Get minimum constant for ARM instruction set that is greator than

Typo on "greater" and these should probably be Doxygen comments since
they're describing a function (starting with '///' is enough). There
are actually quite a few typos in the comments. I also spotted:
"manyu", "aligend" and "critarion" too.

+// produced by rotating an 8-bit value right by and even number

Should be "an even number".

+static uint32_t AlignToARMConstant(uint32_t Value) {

Function names usually start with a lower case letter in LLVM. Also
local variables start with a capital (the basic blocks in particular
here).

+  // Get TLS base address.
+  // First try to get it from the coprocessor
[...]
+  // Next, try to get it from the special address 0xFFFF0FF0

This *really* shouldn't be a dynamic check. Your platform should
define where to get the TLS base from, and not in terms of "are you
feeling lucky, punk". Currently, I believe LLVM uses the mrc form in
all cases.

+  // Get the stack limit from the right offset
+  // add SR0, SR0, offset*4
+  AddDefaultPred(BuildMI(getMBB, DL, TII.get(ARM::ADDri), ScratchReg0)
+                 .addReg(ScratchReg0).addImm(4*TlsOffset)).addReg(0);
+
+  // Get stack limit.
+  // ldr SR0, [sr0]
+  AddDefaultPred(BuildMI(getMBB, DL, TII.get(ARM::LDRi12), ScratchReg0)
+                 .addReg(ScratchReg0).addImm(0));

These two should be foldable into an "ldr SR0, [SR0, #4*TlsOffset]"

+  // push {lr} - Save return address of this function.
+  AddDefaultPred(BuildMI(allocMBB, DL, TII.get(ARM::STMDB_UPD))
+                 .addReg(ARM::SP, RegState::Define)
+                 .addReg(ARM::SP))
+    .addReg(ARM::LR)

This misaligns the stack, which needs to be a multiple of 8 across
public interfaces according to AAPCS. Is __morestack defined to take
precautions for that?

+void
+ARMFrameLowering::adjustForSegmentedStacksThumb(MachineFunction &MF) const {

This looks similar enough to the ARM version that I think it should be
fairly easy to merge them. In most cases you can just use a different
opcode (say t2LDR instead of LDR, this whole file only gets involved
when Thumb2 is available) and leave everything else the same.
Functions often do "Opcode = IsThumb ? ARM::t2Whatever :
ARM::Whatever" just before a BuildMI.

 class ARMFrameLowering : public TargetFrameLowering {
 protected:
+  const ARMBaseTargetMachine &TM;

There are already uses of a TargetMachine in ARMFrameLowering.cpp, and
they didn't need this extra member variable. I'd suggest following
suit.

Cheers.

Tim.