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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:219
+struct ExceptionSectionEntry : public SectionEntry {
+  std::vector<ExceptionInfo> ExceptionTable;
+  bool isDebugEnabled;
----------------
shchenz wrote:
> 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.
agree with shchenz. 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1134
+                            : XCOFF::ExceptionSectionEntrySize32;
+    }
+  }
----------------
line  1128-1134 can change to


```
Offset += (is64Bit()) ? (SymbolEntry.Entries.size() + 1) *
                              XCOFF::ExceptionSectionEntrySize64
                        : (SymbolEntry.Entries.size() + 1) *
                              XCOFF::ExceptionSectionEntrySize32;
```


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1201
+          bool hasExceptEntry = false;
+          for (auto &SymbolEntry : ExceptionSection.ExceptionTable) {
+            if (SymbolEntry.Symbol == Sym.MCSym) {
----------------
map also will improve the code efficiency here without  a loop.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1216
           SymbolTableIndex += 2;
+          if (hasExceptionSection() && hasExceptEntry) {
+            if (is64Bit() && ExceptionSection.isDebugEnabled)
----------------
bool hasExceptionSection() { return (bool) getExceptionSectionSize(); } }
and XCOFFObjectWriter::getExceptionSectionSize()  loop for all ExceptionSection.ExceptionTable.

it is low efficiency here.  I think you can use variable to calculate once at the  begin of the function.

and there are several place calll  hasExceptionSection() in the patch, maybe we can reimplement as 
bool hasExceptionSection() { !ExceptionSection.ExceptionTable.empty();}



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1217
+          if (hasExceptionSection() && hasExceptEntry) {
+            if (is64Bit() && ExceptionSection.isDebugEnabled)
+              SymbolTableIndex += 2;
----------------
Maybe better to add some comment here to explain the following code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134195