[PATCH] D73739: Exception support for basic block sections

[Di Mo via Phabricator via llvm-commits]

modimo added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.h:114
       SmallVectorImpl<CallSiteEntry> &CallSites,
+      Optional<SmallVector<CallSiteRange, 4>> &CallSiteRanges,
       const SmallVectorImpl<const LandingPadInfo *> &LandingPads,
----------------
What's the reason for using Optional here? Does checking for size == 0 suffice?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/WasmException.h:35
       SmallVectorImpl<CallSiteEntry> &CallSites,
+      Optional<SmallVector<CallSiteRange, 4>> &OptCallSiteRanges,
       const SmallVectorImpl<const LandingPadInfo *> &LandingPads,
----------------
SmallVectorImpl for consistency.


================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:298
+        ++MI;
+      MF.getSubtarget().getInstrInfo()->insertNoop(MBB, MI);
+      break;
----------------
Using getNoop this should look something like:
```
  MCInst Noop;
  MF.getSubtarget().getInstrInfo()->getNoop(Noop);
  BuildMI(MBB, MI, DL, MF.getSubtarget().getInstrInfo()->get(Noop.getOpcode()));
```
I would also create a function to encapsulate this. Logically I think this fits here since you want to do this after `MF.assignBeginEndSections();` but before `updateBranches(MF, PreLayoutFallThroughs);` however the naming of `sortBasicBlocksAndUpdateBranches` is now not accurate. Personally I think a rename to `finalizeBasicBlocks` would work but I'll let @snehasish chime in here.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:90-91
+// Emit a 1-byte noop at the specified position.
+void X86InstrInfo::insertNoop(MachineBasicBlock &MBB,
+                              MachineBasicBlock::iterator MI) const {
+  DebugLoc DL;
----------------
Use the generic TargetInstrInfo.h: virtual void getNoop(MCInst &NopInst) const;
Here so that this is extensible to other architectures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73739