[PATCH] D77080: [NFC][XCOFF][AIX] Refactor get/setContainingCsect

[Jason Liu via Phabricator via llvm-commits]

jasonliu marked an inline comment as done.
jasonliu added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5146
               SectionKind::getMetadata());
-          S->setContainingCsect(Sec);
+          S->setCsectMCSectionXCOFF(Sec);
         }
----------------
hubert.reinterpretcast wrote:
> daltenty wrote:
> > It feels like this breaks the constraint you had listed under 2, since the symbol S is not actually the symbol representing the csect (i.e. the qualname symbol). Would we be able to wrap this whole business in a call to an new TLOF function like `TargetLoweringObjectFileXCOFF::getSectionForUndefinedFunction(StringRef)` and just use the qualname symbol we get back instead (so we don't need to call `setCsectMCSectionXCOFF()` seperately)? 
> > 
> > If we can do this we could restrict `setCsectMCSectionXCOFF()` so it can only be called from MCSectionXCOFF's constructor, enforcing the constraint that this only valid for symbols representing csects (and `hasCsectMCSectionXCOFF()` could become `isCsect()` ). 
> Since the symbol is undefined. It must represent the csect. I also would prefer if we can have a stronger correspondence between being a QualName symbol and being the representation of a csect, but I am not sure how much that entails.
I think I'm going to use the same reason why this haven't been done in D72347: we do not have the access to TLOF here.
Aside from that, I had the same debate that you have. The conclusion I came up with is that all the undefined symbol are essentially csects (even though that undefined symbol is actually a label in the other compilation unit). So in here, although we do not have a qualname, this symbol still represents a csect.
I agree with you that it might be better if we have a qualname `.bl .foo[PR]` here when foo is an undefined function. So that we know if we have a qualname, it must be csect. But right now, I don't see an immediate need for that and I kinda want to leave things as it is until there are concrete reasons to change it.  



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77080