[PATCH] D14614: [WinEH] Find root frame correctly in CLR funclets

[Joseph Tremoulet via llvm-commits]

JosephTremoulet added inline comments.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1206-1213
@@ +1205,10 @@
+      unsigned PSPSlotOffset = getPSPSlotOffsetFromSP(MF);
+      MachinePointerInfo NoInfo;
+      MBB.addLiveIn(Establisher);
+      addRegOffset(BuildMI(MBB, MBBI, DL, TII.get(X86::MOV64rm), Establisher),
+                   Establisher, false, PSPSlotOffset)
+          .addMemOperand(MF.getMachineMemOperand(
+              NoInfo, MachineMemOperand::MOLoad, SlotSize, SlotSize));
+      ;
+      // Save the root establisher back into the current funclet's (mostly
+      // empty) frame, in case a sub-funclet or the GC needs it.
----------------
Great, thanks for the pointer.  I added -verify-machineinstrs to the test RUN line, and did need to update this just as you suggested, also was missing the live-in annotation for the parameter.  I added the load and store as MachineMemoryOperands as well to be safe, so now the MIR looks like this:


```
	%RCX<def> = MOV64rm %RCX, 1, %noreg, 40, %noreg; mem:LD8[<unknown>]
	MOV64mr %RSP, 1, %noreg, 40, %noreg, %RCX; mem:Volatile ST8[<unknown>]
```

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1281-1283
@@ +1280,5 @@
+    unsigned PSPSlotOffset = getPSPSlotOffsetFromSP(MF);
+    auto PSPInfo = MachinePointerInfo::getFixedStack(
+        MF, MF.getMMI().getWinEHFuncInfo(Fn).PSPSymFrameIdx);
+    addRegOffset(BuildMI(MBB, MBBI, DL, TII.get(X86::MOV64mr)), StackPtr, false,
+                 PSPSlotOffset)
----------------
Updated as above, now getting


```
	MOV64mr %RSP, 1, %noreg, 40, %noreg, %RSP; mem:Volatile ST8[FixedStack0]
```

Adding volatile seemed right since the GC/unwinder may be looking at this, and since the loads in the funclets aren't annotated with the frame index.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1414
@@ +1413,3 @@
+    // resides at in the main function.
+    UsedSize = getPSPSlotOffsetFromSP(MF) + SlotSize;
+  } else {
----------------
I don't understand what I'd need to tweak.  Do you mean so that funclets can access their copy of the PSPSym?  I'm generating those accesses directly in the prolog emission (the only references are in the prolog), so they aren't using the offset of the frame object -- that object only describes the PSPSym slot in the root function's frame.  Or did you mean to optimize frame size by allocating the PSPSym towards the bottom of the frame?  Or something else entirely?


http://reviews.llvm.org/D14614