[PATCH] D84765: [AIX][XCOFF] change oprand of branch instruction from symbol name to qualify symbol name.

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L added a comment.

As you mentioned in the summary, this patch seems be addressing two orthogonal things:

1. remove SC from MCSectionXCOFF and let it only be a property of MCSymbolXCOFF
2. when branching to an undefined function, branch to qualname instead.

If I am right, I would suggest we split them into two patches.

And for the second patch `when branching to an undefined function, branch to qualname instead`, I think we need to adjust the patch title to a more accurate one[maybe `[AIX][XCOFF] change the operand of branch instruction from symbol name to qualified symbol name for function declarations`]since the scope is actually limited to function declarations only.



================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:50
     QualName->setRepresentedCsect(this);
+    QualName->setStorageClass(XCOFF::C_HIDEXT);
     // A csect is 4 byte aligned by default, except for undefined symbol csects.
----------------
I am wondering why do we want to set a default SC here? It seems redundant.


================
Comment at: llvm/include/llvm/MC/MCSymbolXCOFF.h:38
   void setStorageClass(XCOFF::StorageClass SC) {
-    assert((!StorageClass.hasValue() || StorageClass.getValue() == SC) &&
-           "Redefining StorageClass of XCOFF MCSymbol.");
----------------
It looks like this assertion is still useful to prevent someone from setting SC twice or setting other SC for a same symbol accidentally, if we don't need to set a default SC as I mentioned above.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1705
   MCSymbolXCOFF *GVSym = cast<MCSymbolXCOFF>(getSymbol(GV));
-  GVSym->setStorageClass(
-      TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GV));
----------------
Please update the comment accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84765