[PATCH] D134195: [PowerPC] XCOFF exception section support on the integrated assembler path

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/include/llvm/MC/MCObjectWriter.h:111
+                                 unsigned FunctionSize, bool hasDebug) {
+    report_fatal_error("addExceptionEntry is only supported on "
+                       "XCOFF targets");
----------------
Format here seems strange...


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:213
+struct ExceptionInfo {
+  const MCSymbol *Symbol;
+  unsigned FunctionSize;
----------------
Move this field to `ExceptionSectionEntry` as the key.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:219
+struct ExceptionSectionEntry : public SectionEntry {
+  std::vector<ExceptionInfo> ExceptionTable;
+  bool isDebugEnabled;
----------------
Can we change this to a map? The key is the function symbol(The `Symbol` in `ExceptionInfo` struct) and the value is the `ExceptionInfo` struct which contains all entries for a function. So we don't need to go through all the functions exception table to find a `ExceptionInfo` for a given symbol? I saw too many such loops in your implementation that can be optimized out by using a map.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:357
+                         unsigned FunctionSize, bool hasDebug) override;
+  bool hasExceptionSection() { return (bool) getExceptionSectionSize(); }
+  unsigned getExceptionSectionSize();
----------------
Can we use `!ExceptionTable.empty()` for this? `getExceptionSectionSize()` sounds like too heavy.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:791
+                       CSectionRef.Address + SymbolOffset, SectionIndex,
+                       is64Bit() ? SymbolRef.getVisibilityType()
+                                 : SymbolRef.getVisibilityType() | 0x0020,
----------------
Can we explain why we need to add `0x0020` for 32-bit symbol?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:794
+                       SymbolRef.getStorageClass(),
+                       (is64Bit() && ExceptionSection.isDebugEnabled) ? 3 : 2);
+
----------------
If we are using a map for `ExceptionTable`, we can reuse `writeSymbolEntry()` and the last `writeSymbolAuxCsectEntry()` with legacy codes. Just need to do some special handling for the last parameter `NumberOfAuxEntries` for symbol which has except entries.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1097
+    ExceptionSection.isDebugEnabled = true;
+  for (auto &SymbolEntry : ExceptionSection.ExceptionTable) {
+    if (Symbol == SymbolEntry.Symbol) {
----------------
Can optimize this by using a map.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1114
+  unsigned Size = 0;
+  for (auto &SymbolEntry : ExceptionSection.ExceptionTable) {
+    Size += (is64Bit()) ? (SymbolEntry.Entries.size() + 1) *
----------------
Maybe we need comments here for the `+1`? The first exception entry for a function.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1423
+  for (auto const &SymbolEntry : ExceptionEntry.ExceptionTable) {
+    W.write<uint32_t>(SymbolIndexMap[SymbolEntry.Symbol]);
+    if (is64Bit()) {
----------------
Please add comments here for the first section entry.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section-debug.ll:1
+; RUN: llc -O0 -mtriple=powerpc64-unknown-aix -filetype=obj -o %t.o < %s
+; RUN: llvm-readobj --exception-section %t.o | FileCheck %s --check-prefix=EXCEPT64
----------------
nit: is the `-O0` required?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section-debug.ll:6
+; RUN: llc -O0 -mtriple=powerpc64-unknown-aix -filetype=obj -o %t.o < %s
+; RUN: llvm-readobj --syms %t.o | FileCheck %s --check-prefix=SYMS64
+
----------------
For this test, if it is to test the `Exception Auxiliary Entry` aux symbol, maybe we can only keep one RUN line to only check the symbols. Other test points should be covered in other tests, right?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section.ll:207
+; EXCEPT64-NEXT:    Reason: 2
+; EXCEPT64-NEXT:  }
+
----------------
This is exactly the same with 32-bit? If so, we don't need to make another copy.


================
Comment at: llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations-td.ll:7
 ; RUN:   --ppc-asm-full-reg-names -mcpu=pwr8 < %s | FileCheck %s -check-prefix=AIX
-; RUN: not --crash llc -verify-machineinstrs -mtriple=powerpc64-unknown-aix \
-; RUN:   --ppc-asm-full-reg-names -mcpu=pwr8 --filetype=obj -o /dev/null %s 2>&1 | FileCheck %s -check-prefix=OBJ
----------------
We should not remove the RUN line as it is the only one test for -filetype=obj. Maybe we can modify the check to a simple check, for example, `; OBJ-LABEL: test__trapd_annotation:` to check that the case does not crash in obj mode now?


================
Comment at: llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations-tw.ll:14
-
-; OBJ: LLVM ERROR: emitXCOFFExceptDirective not yet supported for integrated assembler path.
 
----------------
ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134195