[PATCH] D70461: [AIX] Emit TOC entries for ASM printing

[David Tenty via Phabricator via llvm-commits]

daltenty marked 9 inline comments as done.
daltenty added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:111
 
-  void emitTCEntry(const MCSymbol &S) override {
+  void emitTCEntry(const MCSymbol &Entry, const MCSymbol &Target) override {
+    const MCAsmInfo *MAI = Streamer.getContext().getAsmInfo();
----------------
jasonliu wrote:
> daltenty wrote:
> > jasonliu wrote:
> > > daltenty wrote:
> > > > jasonliu wrote:
> > > > > I'm on the fence of this interface change.
> > > > > I could see this interface change makes emitTCEntry a bit cleaner, and it models the TC entry a bit better: a symbol represent the real TC, and a symbol represent the item that TC is trying to store. 
> > > > > But on the other hand, I don't think we are ever getting ".tc A[TC], B", and we always have ".tc A[TC], A" (at least for now). Using the new interface would kinda make that implicit connection lost. 
> > > > If that relationship is something we want to preserve, we could add an assert of the form:
> > > > 
> > > > ```
> > > > assert(Entry.getName().equals(MAI->getSymbolsHaveSMC() ? Target.getUnqualifiedName()  : Target.getName()) && "TOC entries should normally have the same name as their targets")
> > > > ```
> > > Then do we really need two MCSymbol as parameters? I feel like it just complicates the relationship. And one MCSymbol is enough, even on AIX.
> > I think it make sense that we need both, it models the actual TC entry better as you said. And using one also presents difficulties in the AIX case.  
> > 
> > We can't the qualname symbol, because this will not work for the object writing case as we need the value. But if we proceed with just the target symbol we end up rebuilding the same string we already had in the qualname symbol to begin with, since it's the same as the TC CSect (as we would expect).
> > 
> > If we want to maintain the old interface, for the PPC LE target where they are the same symbol, we just create a wrapper where duplicate the argument?
> Maybe calling getUnqualifiedName() again is not that bad. But I'm Okay with what we have right now in the patch. The assert and wrapper does not really make things better. 
Taking another look, I agree. I will update it to just call getUnqualifiedName() again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70461