[PATCH] D73739: Exception support for basic block sections

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added a comment.

Thanks for the comments @MaskRay



================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:145
   MCSymbol *CurExceptionSym = nullptr;
+  DenseMap<const MachineBasicBlock *, MCSymbol *> ExceptionSymbols;
 
----------------
MaskRay wrote:
> Move below CurrentSectionBeginSym and add a comment to make it clear this is for basic block sections.
In light of your comment, I refactored this map to be used for both situations.
This refactoring also removed some code .


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


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:257
 
+  // All landing pads should reside in one section, and hence in one call-site
+  // range.
----------------
MaskRay wrote:
> Seems that this comment can be merged with the next comment. 
This comment was left out from old code. Thanks for catching. Removed.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:485
 
   bool VerboseAsm = Asm->OutStreamer->isVerboseAsm();
 
----------------
MaskRay wrote:
> 
This is existing code copied here. Does it matter that we make the boolean const since it's temporary and we can't take the address/reference of the temporary.


================
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
----------------
MaskRay wrote:
> This comment does not start with a complete sentence.
Thanks again.


================
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
----------------
MaskRay wrote:
> You can place NON-PIC together so that you can use `-NEXT:`
Apparently, `-NEXT` works even without them being together, but I organized the checks.


================
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
----------------
MaskRay wrote:
> The comment markers appear to be misaligned.
Thanks. Fixed here and in the other test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73739