[PATCH] D132146: [PowerPC] XCOFF exception section support on the direct assembler path and exception language and reason code lowering from trap intrinsics

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

Thanks, looks almost good to me. Some minor comments.



================
Comment at: llvm/include/llvm/MC/MCObjectWriter.h:108
 
+  virtual void addExceptionEntry(const MCSymbol *Symbol, MCSymbol *Trap,
+                                 unsigned LanguageCode, unsigned ReasonCode,
----------------
This seems not belonging to this patch?


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:949
 
+void MCAsmStreamer::emitXCOFFExceptDirective(const MCSymbol *Symbol,
+                             MCSymbol *Trap, unsigned Lang, unsigned Reason, 
----------------
format


================
Comment at: llvm/lib/MC/MCStreamer.cpp:1193
 
+void MCStreamer::emitXCOFFExceptDirective(const MCSymbol *Symbol,
+                          MCSymbol *Trap, unsigned Lang, unsigned Reason,
----------------
Please use clang-format to reformat all the changes. The format here seems not right.


================
Comment at: llvm/lib/MC/MCStreamer.cpp:1197
+  llvm_unreachable("emitXCOFFExceptDirective is only supported on "
+                   "XCOFF targets");
+}
----------------
`report_fatal_error` seems like a better choice. `llvm_unreachable` may not stop the program in some configurations.


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:89
+                                               bool hasDebug) {
+  llvm_unreachable("emitXCOFFExceptDirective not yet supported for integrated"
+                   "assembler path.");
----------------
`report_fatal_error`


================
Comment at: llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations-64bit.ll:11
+
+; llc filetype=obj tested in /llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section.ll
+
----------------
This seems not right.

You already added RUN line for obj file type. You can just check: (can not redirect stderr to `/dev/null`)
```
: OBJ: LLVM ERROR: emitXCOFFExceptDirective not yet supported for integrated assembler path.
```


================
Comment at: llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-trap-annotations.ll:11
+
+; llc filetype=obj tested in /llvm/test/CodeGen/PowerPC/aix-xcoff-exception-section.ll
+
----------------
Ditto. add a new runline for obj file type and check the error messages.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132146