[PATCH] D70718: [AIX] Replace ExternalSymbolSDNode with a MCSymbolSDNode for memcpy/memset/memmove

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added a comment.

As Jason suggested, the code duplication can be reduced:

  const auto ProduceDescriptorAndReplaceCallee =
      [&](StringRef FuncName, bool EnsureContainingCsectIsSet,
          const auto &GetStorageClass) {
        auto &Context = DAG.getMachineFunction().getMMI().getContext();
  
        MCSymbolXCOFF *S = cast<MCSymbolXCOFF>(
            Context.getOrCreateSymbol(Twine(".") + Twine(FuncName)));
  
        if (EnsureContainingCsectIsSet && !S->hasContainingCsect()) {
          // On AIX, an undefined symbol needs to be associated with a
          // MCSectionXCOFF to get the correct storage mapping class. In this
          // case, XCOFF::XMC_PR.
          MCSectionXCOFF *Sec = Context.getXCOFFSection(
              S->getName(), XCOFF::XMC_PR, XCOFF::XTY_ER, GetStorageClass(),
              SectionKind::getMetadata());
          S->setContainingCsect(Sec);
        }
  
        // Replace the callee SDNode with the MCSymbolSDNode.
        Callee = DAG.getMCSymbol(S, PtrVT);
        Ops[1] = Callee;
      };
  
  if (isFunctionGlobalAddress(Callee)) {
    GlobalAddressSDNode *G = cast<GlobalAddressSDNode>(Callee);
    const GlobalObject *GO = cast<GlobalObject>(G->getGlobal());
    ProduceDescriptorAndReplaceCallee(
        GO->getName(), GO->isDeclaration(), [&] {
          return TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GO);
        });
  } else if (ExternalSymbolSDNode *const E =
                 dyn_cast<ExternalSymbolSDNode>(Callee)) {
    const char *SymName = E->getSymbol();
  
    // TODO: Remove this when the support for ExternalSymbolSDNode is
    // complete.
    if (strcmp(SymName, "memcpy") == 0 || strcmp(SymName, "memmove") == 0 ||
        strcmp(SymName, "memset") == 0) {
      ProduceDescriptorAndReplaceCallee(SymName, true,
                                        [] { return XCOFF::C_EXT; });
    }
  }



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5337
+
+      if (GO && GO->isDeclaration() && !S->hasContainingCsect()) {
+        // On AIX, an undefined symbol needs to be associated with a MCSectionXCOFF
----------------
`GO` cannot be null or `getName` was called with a null pointer just above.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5339
+        // On AIX, an undefined symbol needs to be associated with a MCSectionXCOFF
+	// to get the correct storage mapping class. In this case, XCOFF::XMC_PR.
+	const XCOFF::StorageClass SC =
----------------
The formatting is still off here. If the comment is merged back into one line, `clang-format` will produce:
```
        // On AIX, an undefined symbol needs to be associated with a
        // MCSectionXCOFF to get the correct storage mapping class. In this
        // case, XCOFF::XMC_PR.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5340
+	// to get the correct storage mapping class. In this case, XCOFF::XMC_PR.
+	const XCOFF::StorageClass SC =
+            TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GO);
----------------
The indentation for this line is affected as well.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5353
+
+    if (ExternalSymbolSDNode *E = dyn_cast<ExternalSymbolSDNode>(Callee)) {
+      const char *SymName = E->getSymbol();
----------------
If the two conditions are mutually exclusive, then this should be an `else if`. If this check is meant to operate on the new `Callee` assigned in the block above, then a comment really should be added.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5355
+      const char *SymName = E->getSymbol();
+      //TODO: Remove this when the support ExternalSymbolSDNode is complete.
+      if (strcmp(SymName, "memcpy") == 0 || strcmp(SymName, "memmove") == 0 ||
----------------
Add a space after the colon and add "for" after "support":
```
// TODO: Remove this when the support for ExternalSymbolSDNode is complete.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5364
+          // On AIX, undefined symbol need to associate with a MCSectionXCOFF to
+          // get the correct storage mapping class. In this case, XCOFF::XMC_PR.
+          MCSectionXCOFF *Sec = Context.getXCOFFSection(
----------------
If this is not gone, please copy the corrected comment text from the upstream-reviewed version above.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5366
+          MCSectionXCOFF *Sec = Context.getXCOFFSection(
+              S->getName(), XCOFF::XMC_PR, XCOFF::XTY_ER, XCOFF::C_EXT,
+              SectionKind::getMetadata());
----------------
What happens if the user is defining `memset` in this file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70718