[PATCH] D70243: Lowering CPI/JTI/BA to assembly

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1757
 
+      if (getSubtargetInfo().getTargetTriple().getObjectFormat() ==
+	  Triple::XCOFF) {
----------------
jasonliu wrote:
> sfertile wrote:
> > Nit: Formatting.
> We could call member function of triple instead of raw comparison:
> TM.getTargetTriple().isOSAIX or TM.getTargetTriple().isOSBinFormatXCOFF
> Same for all the raw comparison below. 
Sorry, but can you point out the formatting issue more explicitly?  And the formatting here is got from `clang-format`.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1757
 
+      if (getSubtargetInfo().getTargetTriple().getObjectFormat() ==
+	  Triple::XCOFF) {
----------------
Xiangling_L wrote:
> jasonliu wrote:
> > sfertile wrote:
> > > Nit: Formatting.
> > We could call member function of triple instead of raw comparison:
> > TM.getTargetTriple().isOSAIX or TM.getTargetTriple().isOSBinFormatXCOFF
> > Same for all the raw comparison below. 
> Sorry, but can you point out the formatting issue more explicitly?  And the formatting here is got from `clang-format`.
Thanks, I will update it.


================
Comment at: llvm/lib/CodeGen/MachineModuleInfo.cpp:123
+      Triple::XCOFF) {
+    MCSymbol *FnEntryPointSym =
+        Context.getOrCreateSymbol("." + Entry.Fn->getName());
----------------
sfertile wrote:
> IIUC we expect that the functions entry point symbol already exists, and has its containing csect properly set. If so it be more appropriate to use `Context.lookupSymbol` and have appropriate error handling if the symbol doesn't exist or if its containing csect is not set.
Thanks, I will add an assertion after we do `lookupSymbol` for `FnEntryPointSym`. And since in `getContainingCsect()`, we already add assertion to test if the csect for the sym exists or not, I will omit the assertion here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70243