[PATCH] D102817: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 2

[Ten Tzen via Phabricator via llvm-commits]

tentzen marked 11 inline comments as done.
tentzen added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1624
+        //   (see MCConstantExpr::create(1,..) in WinException.cpp)
+        //  Ignore SDiv/UDiv because a DIV with Const-0 divisor
+        //  SDiv/UDiv because a DIV with Const-0 divisor
----------------
pengfei wrote:
> duplicated.
good eyes. thanks.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1632
+          if (IsEHa && MI2 != MBB.end() &&
+              (MI2->mayLoadOrStore() || MI2->mayRaiseFPException()))
+            emitNops(1);
----------------
pengfei wrote:
> Check for call instructions?
SEH HW exception will be caught in callee. No need to inject a nop before the call


================
Comment at: llvm/lib/CodeGen/AsmPrinter/WinException.cpp:774
+  if (MMI->getModule()->getModuleFlag("eh-asynch")) {
+    OS.emitInt32(0);
+  } else {
----------------
pengfei wrote:
> Check it in test case?
yes, without this change, tests will fail


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1289
+    return;
+  for (auto MBBI = MF->begin(); MBBI != MF->end(); ++MBBI) {
+    MachineBasicBlock *MBB = &*MBBI;
----------------
pengfei wrote:
> It's more preferred to use `.. E = MF->end(); MBBI != E; ..`
ok.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1293
+    int State = EHInfo->BlockToStateMap[BB];
+    if (BB->getFirstMayFaultInst()) {
+      // Report IP range only for blocks with Faulty inst
----------------
pengfei wrote:
> Any reason this has to be done in `SelectionDAGISel`. If we do it after ISel, we can check `mayRaiseFPException` for FP instructions too.
It's convenient and a natural place to do this table. It's already at the end of DAGISel.
If we are still missing some features, I prefer a follow-up patch. This patch has been tested and proved robust. 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1302-1303
+      MachineInstr *MIb = &*MBBb;
+      if (MIb->isTerminator())
+        continue;
+      BuildMI(*MBB, MBBb, SDB->getCurDebugLoc(),
----------------
pengfei wrote:
> Is there any chance a terminator will raise exception?
> If we don't emit begin/end labels, why adding them to EHInfo by line 1297?
No. terminator won't raise exception.
Good catch, will move them to the top of that block to avoid unused IPToState entry.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1309-1311
+      // insert before (possible multiple) terminators
+      while (MIe->isTerminator())
+        MIe = &*(--MBBe);
----------------
pengfei wrote:
> You can use `getFirstTerminator`.
I think I did it once. the reason getFirstTerminator() not used is that there could be real instructions between 1st and 2nd terminator. it's being a while, don't remember exactly. I think it's better not changing it now. this is minor anyways. 


================
Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:254-256
+      if (Fn && Fn->isIntrinsic() &&
+          (Fn->getIntrinsicID() == Intrinsic::seh_scope_begin ||
+           Fn->getIntrinsicID() == Intrinsic::seh_try_begin))
----------------
pengfei wrote:
> There are many function calls, would be better to simplify to
> ```
> if (Fn && Fn->isIntrinsic()) {
>   Intrinsic::ID IID = Fn->getIntrinsicID();
>   if (IID == ...)
> ```
it does not really matter as duplicated Fn->getIntrinsicID() will be CSEed away


================
Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:270
+    for (auto *SuccBB : successors(BB)) {
+      WI = new WorkItem(SuccBB, State);
+      WorkList.push_back(WI);
----------------
pengfei wrote:
> ditto.
ditto.


================
Comment at: llvm/lib/IR/BasicBlock.cpp:212
 
+const Instruction *BasicBlock::getFirstFaultyInst() const {
+  if (InstList.empty())
----------------
pengfei wrote:
> tentzen wrote:
> > JosephTremoulet wrote:
> > > I didn't notice when commenting on this name earlier that this method is on the IR `BasicBlock` class.  I don't know if there's a better term to use for this than "fault".  Regardless, I think it would be good to make the comment on the declaration very explicit that this checks for loads/stores (which may dereference a null pointer) and calls/invokes (which may propagate exceptions), since I don't think we have a well-defined IR concept for this particular property of those instructions.  (FWIW, The last time I went looking, the closest I could find was the logic in SimplifyCFG that decides how much code it can remove before an unconditional branch to `unreachable`, which was open-coded.)
> > renamed it with getFirstMayFaultInst(). also added more comments in header.  thanks!
> We'd better use machine instruction infos.
this is the style copied from neighborhood code. do you see any problem or potential bug?


================
Comment at: llvm/test/CodeGen/X86/windows-seh-EHa-CppCatchDotDotDot.ll:290
+!1 = !{i32 2, !"eh-asynch", i32 1}
+!2 = !{!"clang version 11.0.0 (https://github.com/llvm/llvm-project 7ee479a760e0a4402b4eb7fb6168768a44f66945)"}
+
----------------
pengfei wrote:
> This is useless and misleading. Ditto for other tests.
this is testing SEH related table and flag is put properly.
which part is misleading?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102817/new/

https://reviews.llvm.org/D102817