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

[Reid Kleckner via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1206-1213
@@ +1205,10 @@
+      unsigned PSPSlotOffset = getPSPSlotOffsetFromSP(MF);
+      addRegOffset(
+          BuildMI(MBB, MBBI, DL, TII.get(X86::MOV64rm)).addReg(Establisher),
+          Establisher, false, PSPSlotOffset);
+      // Save the root establisher back into the current funclet's (mostly
+      // empty) frame, in case a sub-funclet or the GC needs it.
+      addRegOffset(BuildMI(MBB, MBBI, DL, TII.get(X86::MOV64mr)), StackPtr,
+                   false, PSPSlotOffset)
+          .addReg(Establisher);
+    }
----------------
JosephTremoulet wrote:
> Do I need to do anything additional here to capture appropriate dataflow constraints?  The MachineInstrs this generates dump as:
> 
> 
> ```
> 	MOV64rm %RCX, %RCX, 1, %noreg, 40, %noreg
> 	MOV64mr %RSP, 1, %noreg, 40, %noreg, %RCX
> ```
> 
> and I don't know if I need to be marking rsp as a kill, doing something to get it to show up as a LHS, etc.
> 
I think you want to say this for the load into RCX:
  addRegOffset(
            BuildMI(MBB, MBBI, DL, TII.get(X86::MOV64rm), Establisher),
            Establisher, false, PSPSlotOffset);
Using that overload of BuildMI marks it as a def of RCX, which the MachineVerifier cares about.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1273-1275
@@ +1272,5 @@
+    unsigned PSPSlotOffset = getPSPSlotOffsetFromSP(MF);
+    addRegOffset(BuildMI(MBB, MBBI, DL, TII.get(X86::MOV64mr)), StackPtr, false,
+                 PSPSlotOffset)
+        .addReg(StackPtr);
+  }
----------------
JosephTremoulet wrote:
> Similarly, this begets
> 
> 
> ```
> 	MOV64mr %RSP, 1, %noreg, 40, %noreg, %RSP
> ```
> 
> and I'm wondering if the store needs to be marked volatile like C++'s UnwindHelp store is, etc.
I think David was being extra careful by adding volatile. I think he was concerned that backend pass might remove it as it's a store to a stack object that isn't used anywhere else. I'm not convinced it's necessary once we've done prologue insertion after register allocation, but it can't hurt.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1401
@@ +1400,3 @@
+    // resides at in the main function.
+    UsedSize = getPSPSlotOffsetFromSP(MF) + SlotSize;
+  } else {
----------------
I think this can work, you just need to tweak PEI::calculateFrameObjectOffsets() in a subsequent change.


http://reviews.llvm.org/D14614