[PATCH] D66724: [AIX]Emit function descriptor csect in assembly

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L marked 6 inline comments as done.
Xiangling_L added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1712
+  // Emit function descriptor.
+  OutStreamer->SwitchSection(const_cast<MCSectionXCOFF *>(cast<MCSymbolXCOFF>
+                             (CurrentFnDescSym)->getContainingCsect()));
----------------
jasonliu wrote:
> The const_cast is interesting here. It makes me question if we have the right interface for getContainingCsect(). 
> Maybe the getter function should return a non-const MCSectionXCOFF instead? Since SwitchSection could call Section.setIsRegistered(true) which actually modifies that MCSectionXCOFF. Would it be undefined behavior? Please correct me if I'm wrong...
I had this doubt too, we can discussed more in the meeting.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1740
+  // Switch to section to emit TOC-base.
+  OutStreamer->SwitchSection(TOCAnchor);
+}
----------------
jasonliu wrote:
> We would emit the TOC anchor every time even if we don't need it.
> I think we should do the SwitchSection in EmitEndOfAsmFile. We would only emit TOC anchor if there is a function definition in the module. 
> Then a later on patch could implement the emitTCEntry in that same function. 
Yes, it's a better idea to emit in `EmitEndOfAsmFile`. Though a little concern is that I look through this function descriptor patch again, and I think actually we seems don't have to emit `.toc` in this patch, but it makes more sense to put this part together with emit TC entry patch. Since even if this patch does emit `TC0` in function descriptor, we create it by using the enum value `MCSymbolRefExpr::VK_PPC_TOC_TC0`.

And by doing so, we can unwind the emitTCEntry patch from the dependency of this one.  I can do `CHECK-NOT: .tc` and `CHECK-NOT: .toc`, then whichever patch lands later, can update testcases.

But of course, if you still think we want to have `.toc` emitted in this patch, I would be happy to update the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66724