[PATCH] D51524: [ARM64] [Windows] Handle funclets

Tue Oct 30 15:07:55 PDT 2018

ssijaric added inline comments.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:645
       Subtarget.isCallingConvWin64(MF.getFunction().getCallingConv());
-  unsigned FixedObject = IsWin64 ? alignTo(AFI->getVarArgsGPRSize(), 16) : 0;
+  // Var args are accounted for in the containting function, so don't
+  // include them for funclets.
----------------
rnk wrote:
> "containing"
Will fix in the updated test case.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:909

+static bool isFuncletReturnInstr(MachineInstr &MI) {
+  switch (MI.getOpcode()) {
----------------
MatzeB wrote:
> Can be `const MachineInstr&`
Will change in the updated patch.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:934-935
   }
-  int NumBytes = MFI.getStackSize();
-  const AArch64FunctionInfo *AFI = MF.getInfo<AArch64FunctionInfo>();
+  int NumBytes = IsFunclet ? (int)getWinEHFuncletFrameSize(MF)
+                           : MFI.getStackSize();
+  AArch64FunctionInfo *AFI = MF.getInfo<AArch64FunctionInfo>();
----------------
MatzeB wrote:
> Out of interest: How is `getWinEHFuncletFrameSize()` different from `MachineFrameIndo::getStackSize()`?
The stack size (getStackSize()) includes all the locals in its size calculation.  We don't include these locals when computing the stack size of a funclet, as they are allocated in the parent's stack frame and accessed via the frame pointer from the funclet.  Otherwise, we would be double allocating stack space in the funclet that we don't need.  We only save the callee saved registers in the funclet, which are really the callee saved registers of the parent function, including the funclet.   Perhaps, only the non-volatile registers that the funclet modifies should be saved/restored in the funclet prologue/epilogue, but we don't keep track of these separately.  

I will document this.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1182
+            "Funclets should only be present on Win64");
+        UseFP = true;
       } else {
----------------
MatzeB wrote:
> Add a comment explaining why you need a frame pointer for funclet?
> Knowing nothing about funclets myself I wonder why the normal rules for determining whether we need FP are not enough here...
Will do in the updated patch.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1638
+
+  // Create an UnwidHelp object.
+  int UnwindHelpFI =
----------------
majnemer wrote:
> UnwidHelp -> UnwindHelp?
Will fix in the updated patch.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1639-1641
+  int UnwindHelpFI =
+      MFI.CreateStackObject(/*size*/8, /*alignment*/16, false);
+  EHInfo.UnwindHelpFrameIdx = UnwindHelpFI;
----------------
majnemer wrote:
> Should this be moved after the terminator check?
Yes, it's definitely a better place for it.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1652
+  // function.
+  DebugLoc DL = MBB.findDebugLoc(MBBI);
+  RS->enterBasicBlock(MBB);
----------------
MatzeB wrote:
> My understanding is that all of this stuff is just compiler generated extra instructions? In that case I recommend using an empty debug location instead (I know other places of the code get this wrong too...)
Will fix in the updated patch.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1662
+
+// For Win64 AArch64 EH, the offset to the Unwind object is from the SP before
+// the update.  This is easily retrieved as it is exactly the offset that is set
----------------
MatzeB wrote:
> Use doxygen `///` comments.
Will fix in the updated patch.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1675-1679
+// Patch in zero for now.  Haven't encountered any problems yet.
+unsigned AArch64FrameLowering::getWinEHParentFrameOffset(
+    const MachineFunction &MF) const {
+  return 0;
+}
----------------
majnemer wrote:
> majnemer wrote:
> > majnemer wrote:
> > > This is used to populate `dispFrame` in `_s_HandlerType` IIRC. IIUC, you will have issues with certain catch handlers without this properly populated.
> > Ah, it could be that `dispFrame` is not needed for ARM64. In that case, WinException.cpp should not call `getWinEHParentFrameOffset` and the `ParentFrameOffset` should not be emitted as it will add padding between the HandlerType elements.
> Hmmm... So after a bunch of poking around with ARM64 CL.exe, I can confirm that they are allocating the `dispFrame` field and always fill it with zero!
Thanks for double checking!!  Will leave it as is.

================
Comment at: lib/Target/AArch64/AArch64MCInstLower.cpp:256-268
+
+  switch (OutMI.getOpcode()) {
+  case AArch64::CATCHRET:
+    OutMI = MCInst();
+    OutMI.setOpcode(AArch64::RET);
+    OutMI.addOperand(MCOperand::createReg(AArch64::LR));
+    break;
----------------
MatzeB wrote:
> Why is this in MCInstLower and not AsmPrinter.cpp? Maybe you can even specify it in tablegen (see for example ARMs `LDMIA_RET` and similar)
X86 did the expansion in X86MCInstLower.cpp.  I would keep this way in the current patch, and look into tablegen later.  I'll add a comment.

================
Comment at: test/CodeGen/AArch64/wineh-funclets.ll:60
+
+try.cont:                                         ; No predecessors!
+  store i32 0, i32* %retval, align 4
----------------
MatzeB wrote:
> unused basic block?
The block is generated by clang, and SImplifyCFG removes it.  I'll come up with a better test case.

================
Comment at: test/CodeGen/AArch64/wineh-funclets.ll:76
+
+attributes #0 = { noinline norecurse optnone "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { noreturn }
----------------
MatzeB wrote:
> You should be able to drop most of these attributes.
Will update the test case.

================
Comment at: test/CodeGen/AArch64/wineh-funclets.ll:79-83
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"wchar_size", i32 2}
+!1 = !{!"clang version 8.0.0 (http://llvm.org/git/clang.git b4ce07ad3aea023c81f4f6dc8119b56328ae1da7) (http://llvm.org/git/llvm.git 34ec5a2e299dd5c91c5525878afcdcb99b6b3db1)"}
----------------
MatzeB wrote:
> I'm pretty sure you can drop these lines from the test
Will update the test case.

Repository:
  rL LLVM

https://reviews.llvm.org/D51524