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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

I don't really know how exception handling on windows works or what a funclet is. So here's just a couple style comments and questions



================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:909
 
+static bool isFuncletReturnInstr(MachineInstr &MI) {
+  switch (MI.getOpcode()) {
----------------
Can be `const MachineInstr&`


================
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>();
----------------
Out of interest: How is `getWinEHFuncletFrameSize()` different from `MachineFrameIndo::getStackSize()`?


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1182
+            "Funclets should only be present on Win64");
+        UseFP = true;
       } else {
----------------
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...


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1648
+
+  if (MBBI->isTerminator())
+    return;
----------------
In case the previous loop ends with `MBBI == MBB.end()` all the following tests will be undefined behavior.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1652
+  // function.
+  DebugLoc DL = MBB.findDebugLoc(MBBI);
+  RS->enterBasicBlock(MBB);
----------------
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...)


================
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
----------------
Use doxygen `///` comments.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:1668-1672
+  const MachineFrameInfo &MFI = MF.getFrameInfo();
+  LLVM_DEBUG(dbgs() << "Offset from SP for " << FI << " is "
+                    << MFI.getObjectOffset(FI) << "\n");
+  FrameReg = AArch64::SP;
+  return MFI.getObjectOffset(FI);
----------------
doesn't this change the behavior for non-funclets too? If yes this needs to go into a separate patch and needs some comments why.


================
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;
----------------
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)


================
Comment at: test/CodeGen/AArch64/wineh-funclets.ll:60
+
+try.cont:                                         ; No predecessors!
+  store i32 0, i32* %retval, align 4
----------------
unused basic block?


================
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 }
----------------
You should be able to drop most of these attributes.


================
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)"}
----------------
I'm pretty sure you can drop these lines from the test


Repository:
  rL LLVM

https://reviews.llvm.org/D51524