[PATCH] D73739: Exception support for basic block sections

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

I have some understanding with this patch now. Continuously pondering... Here are some comments.



================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:145
   MCSymbol *CurExceptionSym = nullptr;
+  DenseMap<const MachineBasicBlock *, MCSymbol *> ExceptionSymbols;
 
----------------
Move below CurrentSectionBeginSym and add a comment to make it clear this is for basic block sections.


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:245
+  // association is found.
+  MCSymbol *getExceptionSym(const MachineBasicBlock *MBB) {
+    auto R = ExceptionSymbols.find(MBB);
----------------
Move this above, next to the other overload.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:227
+///
+///   - With -function-sections, all call-sites are grouped into one
+///     call-site-range corresponding to the function section.
----------------
The -fno-function-sections case is similar to -ffunction-sections, so there is probably no need to mention function sections.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:257
 
+  // All landing pads should reside in one section, and hence in one call-site
+  // range.
----------------
Seems that this comment can be merged with the next comment. 


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:418
   SmallVector<CallSiteEntry, 64> CallSites;
-  computeCallSiteTable(CallSites, LandingPads, FirstActions);
+  SmallVector<CallSiteRange, 4> CallSiteRanges;
+  computeCallSiteTable(CallSites, CallSiteRanges, LandingPads, FirstActions);
----------------
Probably worth mentioning that normally there is exactly one CallSiteRange. With -fbasic-block-sections, there is one for each cluster of basic block sections.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:485
 
   bool VerboseAsm = Asm->OutStreamer->isVerboseAsm();
 
----------------



================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:494
+    if (HaveTTData) {
+      // here and the amount of padding before the aligned type table. The
+      // assembler must sometimes pad this uleb128 or insert extra padding
----------------
This comment does not start with a complete sentence.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:502
+
+    // The Action table preciesly follows the call-site table. So we emit the
+    // label difference from here (start of the call-site table for SJLJ and
----------------
typo: preciesly

Probably juse: `The Action table follows the call-site table`


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:568
+
+    // Find the call-site range which includes the landing pads.
+    const CallSiteRange *LandingPadRange = nullptr;
----------------
Probably worth mentioning that there is only one basic block section having landing pads.


================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:298
+        ++MI;
+      MF.getSubtarget().getInstrInfo()->insertNoop(MBB, MI);
+      break;
----------------
rahmanl wrote:
> modimo wrote:
> > 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.
> Great suggestion.
> I added a new function avoidZeroOffsetLandingPad which can also be used in MachineFunctionSpliter.
Where will avoidZeroOffsetLandingPad used elsewhere? 


================
Comment at: llvm/test/CodeGen/X86/gcc_except_table_bb_sections.ll:10
+; CHECK-LABEL:    .Lfunc_begin0:
+; CHECK-NON-PIC:    .cfi_personality 3, __gxx_personality_v0
+; CHECK-PIC:        .cfi_personality 155, DW.ref.__gxx_personality_v0
----------------
You can place NON-PIC together so that you can use `-NEXT:`


================
Comment at: llvm/test/CodeGen/X86/gcc_except_table_bb_sections.ll:80
+; CHECK-NEXT:           .uleb128 .Ltmp0-.Lfunc_begin0   # >> Call Site 1 <<
+; CHECK-NEXT:           .uleb128 .Ltmp1-.Ltmp0          #   Call between .Ltmp0 and .Ltmp1
+; CHECK-NEXT:           .uleb128 .Ltmp2-main.2      #     jumps to .Ltmp2
----------------
The comment markers appear to be misaligned.


================
Comment at: llvm/test/CodeGen/X86/gcc_except_table_bb_sections.ll:12
+; CHECK-PIC:        .cfi_personality 155, DW.ref.__gxx_personality_v0
+; CHECK-NON-PIC:    .cfi_lsda 3, .Lexception0
+; CHECK-PIC:        .cfi_lsda 27, .Lexception0
----------------
rahmanl wrote:
> MaskRay wrote:
> > personality/lsda encodings are difficult to remember. Worth a comment like `DW_EH_PE_pcrel | DW_EH_PE_indirect | DW_EH_PE_sdata4`
> You're right, but the LSDA encoding is emitted by MCStreamer::emitCFILsda and I don't see any comment functionality there.
I mean a test file comment, not in CHECK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73739