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

[Xiangling_L]

Summary

For memcpy/memset/memmove, replace ExternalSymbolSDNode with a MCSymbolSDNode, which have a prefix dot before function name as entry point symbol.

[jasonliu]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5472–5473

Are we assuming when entering here, Callee could only be "isFunctionGlobalAddress" and "ExternalSymbolSDNode"?
If so, an assertion here would be necessary. If not, we would end up replace the callee with a (MCSymbolXCOFF)nullptr, which would be wrong.

5476

The two if statement inside of "Subtarget.isAIXABI()" have very similar content. It would be great if we could do something to common things up.

5478

I still kinda prefer this to be in an assert to begin with.
But if we decide that we want this expensive pattern matching to run in non-assert build as well, then we could keep this way.
Is this some code that we are going to remove later when we think we covered all the cases? If so, do we want to leave a TODO here?

5482

nit: formatting off here.

5487

nit: formatting off here.

[Xiangling_L]

Rebase on latest master;
Address 1st round reviews;

[Xiangling_L]

Minor changes

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5472–5473

Good point. The Callee entering here won't only be isFunctionGlobalAddress and ExternalSymbolSDNode.

5476

I would prefer to make it cleaner as well, but when I take a closer look, though each part goes through similar process, each step is kinda different. Especially, for external symbol sdnode, we only cover three cases so far. All those make it a little bit tricky to common things up. I would appreciate that if you give some more detailed thoughts about how to common things up here.

5478

I can leave this open for a while to get more inputs and I will add a TODO as you suggested.

[hubert.reinterpretcast]

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; });
  }
}

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5219

GO cannot be null or getName was called with a null pointer just above.

5221

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.

5222

The indentation for this line is affected as well.

5235

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.

5237

Add a space after the colon and add "for" after "support":

// TODO: Remove this when the support for ExternalSymbolSDNode is complete.

5246

If this is not gone, please copy the corrected comment text from the upstream-reviewed version above.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5248

What happens if the user is defining memset in this file?

[sfertile]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5238

I think there will be a handful of other symbols we will need to white list for now as well (not needed in this patch, but sooner rather then later). I suggest we add a function that determines if the name an ExternalSDNode refers to is one we support by using a StringSwitch of the accepted names. When a name is not a supported/expected symbol then we can dump the SDNode as part of the debug tracing, and fatal error.

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-external-sym-sdnode-lowering.ll

12

XL calls .___memmove (with three underscores) with no NOP to implement both memcpy and memmove. This is implemented in libc as an extended operation (with no TOC dependency). This is also the case with .___memset for memset.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5238

There are other libc extended operations that don't seem to come through as an ExternalSDNode by default, such as for memcmp. @sfertile, do you have an suggested approach for this?

[Xiangling_L]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5235

Thanks for mentioning that, but in LLVM coding standard: Don’t use else after a return, so I didn't use else if here.

5238

I am not sure if I understand what you said correctly, but I think for those SDNode which doesn't come through as an ExternalSDNode will fall into GlobalAddressSDNode, for which we already add "." as a prefix for function entry point in previous commit?

If I am wrong, can you state your concern more clearly?

5248

I did some test, and I think it's worth bringing up the findings in the srum, but I will document what I found here.

[test_user_define_memcpy.c]
int memcpy ( void * destination, int num ) { return 3; }

typedef struct cpy_array {
  int array[20];
} cpy;

int main() {
  cpy A = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,16};
  cpy B = A;
  return B.array[0];
}

1. XLC on AIX with no-opt:

XLC doesn't synthesize load & store to memcpy, the result is correct =1.

2. GCC on AIX with no-opt:

GCC in this case didn't pick up library version memcpy, and the result was some random garbage value.

3. LLVM with powerpc64le-unknown-linux-gnu target with no-opt:

the same as above GCC case.

So I think the following questions are:

1. Is GCC and LLVM's behavior in this case a bug?
2. Do we want to follow GCC's behavior?
3. If the answer of 2) is yes, how do we want to achieve that, especially when the definition of memcpy is put after callInst of ExternalSymbolSDNode, like the following:

typedef struct cpy_array {
  int array[20];
} cpy;

int main() {
  cpy A = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,16};
  cpy B = A;
  return B.array[0];
}

int memcpy ( void * destination, int num ) { return 3; }

llvm/test/CodeGen/PowerPC/aix-external-sym-sdnode-lowering.ll

12

Thank you for bringing this up, we discussed it in the scrum today, and it's preferred to defer this to a performance related change when we identify more similar cases.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5235

Exactly how does that part of the coding standard apply here? This if is reachable after the previous if block has been entered. Again, the previous if block modifies Callee. Is this check meant to operate upon the modified Callee?

5238

This has to do with mapping a call to memcmp to the entry point symbol named .___memcmp.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5248

Regardless of whether the user-defined or the system-defined version gets called (I believe either is okay), we should make sure that the lookup and possible creation of the symbol here interacts reasonably with the definition or a user-declaration that changes the symbol properties (such as marking the function weak).

[Xiangling_L]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5235

I see what you are talking about here. I refactored the code and will update the patch soon.

5238

It seems also belong to the follow-up performance patch. But I am also curious that any suggestion of it? @sfertile

5249

@hubert.reinterpretcast @cebowleratibm @sfertile @jasonliu Since we didn't get the chance to talk about this in the scrum, I am thinking I can put some of my thoughts here for discussion.

1. I would treat GCC and LLVM 's behavior as a bug in this case. The compiler shouldn't synthesize to a memcpy which conflicts with user-defined memcpy and lead to some undefined behavior. The correct behavior seems should be when the compiler detect a user-defined/user-declared memcpy, it should give up the synthesis and keep original load and store.

Difficulties: We need a way to tell if there is a user-declared/user-defined memcpy in the translation unit when the compiler make the decision that if we want to replace a bunch of load and store with memcpy ExternalSymbolSDNode.

2. On the other hand, if we want to follow GCC's behavior and try to pick up user-defined/user-declared memcpy to replace original bunch of load and store, we also need to way to tell if there is a user-declared/user-defined memcpy in the translation unit, based on which we set Csect accordingly.

In summary, it seems a way to tell if there is a user-declared/user-defined memcpy in the translation unit is a must, i.e. if there is a way to tell if there exists a memcpy GlobalObject. But so far I got no luck to figure out how to do that. Anyone has any thoughts about that?

[Xiangling_L]

Refactor the code

[Xiangling_L]

Clean the formatting issue

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5249

The LLVM and GCC behaviour is acceptable. Under C90 subclause 7.1.3, a user definition of memcpy leads to undefined behaviour. At the assembly and object generation level, we can handle cases where the user leaves memcpy as a function in a somewhat reasonable manner. If the user defines memcpy as an object, then we will preferably have a way to keep both memcpy symbols around even if the resulting assembly or object file is not exactly valid.

As for finding out whether the user has defined memcpy, I think we can look into llvm::Module::getNamedValue.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5125

Is there a rationale to the ordering here? I don't see a semantic or lexical reason for the current ordering. Ordering helps when comparing lists.

5153

The name of this reads like an operation. It is not clear what the return value is (but it does return a value). Is this more getAIXFuncEntryPointSymbol?

5172

I think this should be an MVT to match the return type.

5207–5209

The assert is unnecessary. cast does not return null pointers.

5222

Do you mean that there is no name?

llvm/test/CodeGen/PowerPC/aix-external-sym-sdnode-lowering.ll

41

The test does not cover the larger set of strings that you whitelisted. I'm not sure if that is intentional.

[Xiangling_L]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5125

Thanks, I will order them alphabetically.

5249

Do you mean we don't want to either follow LLVM and GCC's behavior as Under C90 subclause 7.1.3, a user definition of memcpy leads to undefined behavior or not synthesize memcpy but introduce a third way to handle this by generating invalid assembly or object files? Can you state a little bit clearly why we want to do this rather than following LLVM and GCC? And how does invalid assmbly or object file will possibly look like if take memcpy for example?

llvm/test/CodeGen/PowerPC/aix-external-sym-sdnode-lowering.ll

41

Thanks for mentioning that, I was planning to update the testcase/add more testcases when the above ExternalSymbolSDNode undefined behavior discussion is closed.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5249

It is okay to synthesize calls to memcpy expecting that the user did not define it themselves; however, to encode such calls when the symbol is defined in the current translation unit as a variable could be problematic (although less so if the symbols are clearly distinguished, and with the final XCOFF, the object and the function entry point would be distinctly named). As for your question regarding why we are not following LLVM and GCC, I have not said suggested anything that contradicts the LLVM and GCC behaviour as you have described it. The implication in what I said is that the goal is to not trigger an internal compiler error. It seems the scope for which we need to be concerned is already limited by Clang:

error: redefinition of 'memcpy' as different kind of symbol

I guess a problematic case could still be presented through LLVM IR.

[Xiangling_L]

Update the pr to address user-defined/user-declared lib function issue;
Add more testcases;

[Xiangling_L]

ping.

[jasonliu]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5204

This is returning a GlobalValue, which means it could also be a global variable. If so, I don't think we want to enter this branch.

llvm/test/CodeGen/PowerPC/aix-external-sym-sdnode-lowering.ll

2

do we care to put -mcpu=pwr4 in this test case?

llvm/test/CodeGen/PowerPC/aix-user-defined-memcpy.ll

8

Maybe it's worth to explain what this test is for? For example, to make sure under this circumstances we would still set up the right memcpy symbol to call the user defined memcpy instead of external reference memcpy.
The information are all in the review comments which is good, but it would be great if people don't need to dig up the original review to find them.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5201–5205

Add "a" before "user-defined".

5201–5205

Both instances of "user-defined" should be "user-declared".

5202

Add "the" before "user-defined".

5205

dyn_cast can return a null pointer.

5212

This code does not need to be inside an else block.

5217

This does not need to be inside an else block.

llvm/test/CodeGen/PowerPC/aix-user-defined-memcpy.ll

21

This test should preferably check the symbol table for a .o file and the relocation associated with the call.

[Xiangling_L]

llvm/test/CodeGen/PowerPC/aix-external-sym-sdnode-lowering.ll

2

I didn't add -mcpu=pwr4 because not all ExternalSDNode we want to test can be generated under a specific cpu level. And I think this testcase is more focused on, if xxx` ExternalSymbolSDNode synthesized by compiler, we are able to map it to .xxx. So it doesn't really matter to add cpu level as well.

llvm/test/CodeGen/PowerPC/aix-user-defined-memcpy.ll

8

Good point, I will add some comments in the testcase.

21

Good point, I will add it in.

[Xiangling_L]

Address the comments;

[Xiangling_L]

Because the best way to test which memcpy version we pick when there exists a user-declared one, we need to test the relocation of the call. So this patch would be moved to needs revision section, waiting for relocation patch to land first.

[Xiangling_L]

Update two testcases;

[Xiangling_L]

provide full context for diff;

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5122

There is no demonstration that we should expect to encounter __floatdisf. Should we retain it in this list? Similarly for other cases not present in the test.

5155

Sorry for missing this earlier. Would it be better to name this getAIXFuncEntryPointSymbolSDNode?

5173

Remove this comment (it no longer fits here).

5206

I find it very jarring when the next statement after a closing brace is not at the same indentation level (and similarly when the last statement before a closing brace is not indented just one level in).

5209

This if does not need braces. I would much rather spend the braces on the if statement above.

llvm/test/CodeGen/PowerPC/aix-user-defined-memcpy.ll

56

Use --expand-relocs to get past this message.

[jasonliu]

llvm/test/CodeGen/PowerPC/aix-user-defined-memcpy.ll

56

Testing for this error does not make sense. I'm not sure if we plan to support unexpanded relocation anytime soon.
If you want to test relocation, you should add in --expand-relocs as one of your llvm-readobj option.
But currently the relocation patch is under review/revision, you would not get any relocation info, and it does not make sense for you to test something that's "invalid".
I'm not sure if you could do any better than a TODO here.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5122

Same situation with __floatdidf on the new diff.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5211

The two ifs can be folded using dyn_cast_or_null.

llvm/test/CodeGen/PowerPC/aix-user-defined-memcpy.ll

3

There should be no need to pipe into llvm-readobj. Place the split this into separate RUN lines between the .o generation and the inspection of the .o.

6

You only need to produce the .o file once (in this case, on the first RUN line of the file).

32

Replace "a.o file" with "a .o file".

35

Use the [[:space:]] formulation (@DiggerLin can help you) so we can allow the symbol table index to change.

54

Add a 32-SYM-NOT against further symbols named .memcpy.

[Xiangling_L]

delete extra ExternalSymSDNode in code;
modify func name;
update the testcase;

[jasonliu]

LGTM.

[hubert.reinterpretcast]

The test needs some minor changes that can be fixed on the commit. LGTM otherwise.

llvm/test/CodeGen/PowerPC/aix-user-defined-memcpy.ll

7

You need --relocs in addition to --expand-relocs.

55

This needs a comment explaining that we are expecting to have the test fail when the support for relocations land. This would need to be Relocation{{[[:space:]]}} to avoid matching on Relocations.

[Xiangling_L]

A bug is caught that IFunc is not supported on AIX, so we shouldn't have cast a GlobalValue to a GlobalObject for all PPC target.

I added an assertion about this, and prefer reviewers to take one more look and approve it if no concern raised. I should have caught this earlier, sorry for the inconvenience.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5187–5193

Do you mean that GV must be a GlobalIFunc or must not be a GlobalIFunc?

[Xiangling_L]

minor change

[Xiangling_L]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5187–5193

Thanks for pointing this out, I messed it up when I moved code around. Fixed.

[hubert.reinterpretcast]

LGTM.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

static unsigned getCallOpcode(bool isIndirectCall, bool isPatchPoint,
   // will rewrite the nop to be a load of the TOC pointer from the linkage area
   // into gpr2.
   if (Subtarget.isAIXABI() || Subtarget.is64BitELFABI())
     return callsShareTOCBase(&Caller, Callee, TM) ? PPCISD::CALL
                                                   : PPCISD::CALL_NOP;
 
   return PPCISD::CALL;
 }
+
+static bool isValidAIXExternalSymSDNode(StringRef SymName) {
+  return StringSwitch<bool>(SymName)
+      .Cases("__divdi3", "__fixunsdfdi", "__floatundidf", "__floatundisf",
+             "__moddi3", "__udivdi3", "__umoddi3", true)
+      .Cases("ceil", "floor", "memcpy", "memmove", "memset", "round", true)
+      .Default(false);
+}
+
 static SDValue transformCallee(const SDValue &Callee, SelectionDAG &DAG,
                                const SDLoc &dl, const PPCSubtarget &Subtarget) {
   if (!Subtarget.usesFunctionDescriptors() && !Subtarget.isELFv2ABI())
     if (SDNode *Dest = isBLACompatibleAddress(Callee, DAG))
       return SDValue(Dest, 0);
 
   // Returns true if the callee is local, and false otherwise.
   auto isLocalCallee = [&]() {
     const GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee);
     const Module *Mod = DAG.getMachineFunction().getFunction().getParent();
     const GlobalValue *GV = G ? G->getGlobal() : nullptr;
 
     return DAG.getTarget().shouldAssumeDSOLocal(*Mod, GV) &&
            !dyn_cast_or_null<GlobalIFunc>(GV);
   };
 
   // The PLT is only used in 32-bit ELF PIC mode.  Attempting to use the PLT in
   // a static relocation model causes some versions of GNU LD (2.17.50, at
   // least) to force BSS-PLT, instead of secure-PLT, even if all objects are
   // built with secure-PLT.
   bool UsePlt =
       Subtarget.is32BitELFABI() && !isLocalCallee() &&
       Subtarget.getTargetMachine().getRelocationModel() == Reloc::PIC_;
 
-  if (isFunctionGlobalAddress(Callee)) {
-    const GlobalAddressSDNode *G = cast<GlobalAddressSDNode>(Callee);
-    if (!Subtarget.isAIXABI())
-      return DAG.getTargetGlobalAddress(G->getGlobal(), dl,
-                                        Callee.getValueType(), 0,
-                                        UsePlt ? PPCII::MO_PLT : 0);
-
-    // On AIX, direct function calls reference the symbol for the function's
-    // entry point, which is named by prepending a "." before the function's
-    // C-linkage name.
+  const auto getAIXFuncEntryPointSymbolSDNode =
+      [&](StringRef FuncName, bool IsDeclaration,
+          const XCOFF::StorageClass &SC) {
         auto &Context = DAG.getMachineFunction().getMMI().getContext();
 
-    const GlobalObject *GO = cast<GlobalObject>(G->getGlobal());
         MCSymbolXCOFF *S = cast<MCSymbolXCOFF>(
-        Context.getOrCreateSymbol(Twine(".") + Twine(GO->getName())));
+            Context.getOrCreateSymbol(Twine(".") + Twine(FuncName)));
 
-    if (GO && GO->isDeclaration() && !S->hasContainingCsect()) {
+        if (IsDeclaration && !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.
-      const XCOFF::StorageClass SC =
-          TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GO);
-      MCSectionXCOFF *Sec =
-          Context.getXCOFFSection(S->getName(), XCOFF::XMC_PR, XCOFF::XTY_ER,
-                                  SC, SectionKind::getMetadata());
+          MCSectionXCOFF *Sec = Context.getXCOFFSection(
+              S->getName(), XCOFF::XMC_PR, XCOFF::XTY_ER, SC,
+              SectionKind::getMetadata());
           S->setContainingCsect(Sec);
         }
 
-    EVT PtrVT = DAG.getTargetLoweringInfo().getPointerTy(DAG.getDataLayout());
+        MVT PtrVT =
+            DAG.getTargetLoweringInfo().getPointerTy(DAG.getDataLayout());
         return DAG.getMCSymbol(S, PtrVT);
+      };
+
+  if (isFunctionGlobalAddress(Callee)) {
+    const GlobalAddressSDNode *G = cast<GlobalAddressSDNode>(Callee);
+    const GlobalValue *GV = G->getGlobal();
+
+    if (!Subtarget.isAIXABI())
+      return DAG.getTargetGlobalAddress(GV, dl, Callee.getValueType(), 0,
+                                        UsePlt ? PPCII::MO_PLT : 0);
+
+    assert(!isa<GlobalIFunc>(GV) && "IFunc is not supported on AIX.");
+    const GlobalObject *GO = cast<GlobalObject>(GV);
+    const XCOFF::StorageClass SC =
+        TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GO);
+    return getAIXFuncEntryPointSymbolSDNode(GO->getName(), GO->isDeclaration(),
+                                            SC);
   }
 
-  if (ExternalSymbolSDNode *S = dyn_cast<ExternalSymbolSDNode>(Callee))
-    return DAG.getTargetExternalSymbol(S->getSymbol(), Callee.getValueType(),
+  if (ExternalSymbolSDNode *S = dyn_cast<ExternalSymbolSDNode>(Callee)) {
+    const char *SymName = S->getSymbol();
+    if (!Subtarget.isAIXABI())
+      return DAG.getTargetExternalSymbol(SymName, Callee.getValueType(),
                                          UsePlt ? PPCII::MO_PLT : 0);
 
+    // If there exists a user-declared function whose name is the same as the
+    // ExternalSymbol's, then we pick up the user-declared version.
+    const Module *Mod = DAG.getMachineFunction().getFunction().getParent();
+    if (const Function *F =
+            dyn_cast_or_null<Function>(Mod->getNamedValue(SymName))) {
+      const XCOFF::StorageClass SC =
+          TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(F);
+      return getAIXFuncEntryPointSymbolSDNode(F->getName(), F->isDeclaration(),
+                                              SC);
+    }
+
+    // TODO: Remove this when the support for ExternalSymbolSDNode is complete.
+    if (isValidAIXExternalSymSDNode(SymName)) {
+      return getAIXFuncEntryPointSymbolSDNode(SymName, true, XCOFF::C_EXT);
+    }
+
+    report_fatal_error("Unexpected ExternalSymbolSDNode: " + Twine(SymName));
+  }
+
   // No transformation needed.
   assert(Callee.getNode() && "What no callee?");
   return Callee;
 }
 
 static SDValue getOutputChainFromCallSeq(SDValue CallSeqStart) {
   assert(CallSeqStart.getOpcode() == ISD::CALLSEQ_START &&
          "Expected a CALLSEQ_STARTSDNode.");
 
   // The last operand is the chain, except when the node has glue. If the node
   // has glue, then the last operand is the glue, and the chain is the second
   // last operand.
   SDValue LastValue = CallSeqStart.getValue(CallSeqStart->getNumValues() - 1);
   if (LastValue.getValueType() != MVT::Glue)
     return LastValue;
 
   return CallSeqStart.getValue(CallSeqStart->getNumValues() - 2);
 }
 
 // Creates the node that moves a functions address into the count register
 // to prepare for an indirect call instruction.
 static void prepareIndirectCall(SelectionDAG &DAG, SDValue &Callee,
                                 SDValue &Glue, SDValue &Chain,
                                 const SDLoc &dl) {
   SDValue MTCTROps[] = {Chain, Callee, Glue};
   EVT ReturnTypes[] = {MVT::Other, MVT::Glue};
   Chain = DAG.getNode(PPCISD::MTCTR, dl, makeArrayRef(ReturnTypes, 2),
                       makeArrayRef(MTCTROps, Glue.getNode() ? 3 : 2));
   // The glue is the second value produced.
   Glue = Chain.getValue(1);
 }
 
 static void prepareDescriptorIndirectCall(SelectionDAG &DAG, SDValue &Callee,
                                           SDValue &Glue, SDValue &Chain,
                                           SDValue CallSeqStart,
                                           ImmutableCallSite CS, const SDLoc &dl,
                                           bool hasNest,
                                           const PPCSubtarget &Subtarget) {
assert(CallOpc == PPCISD::TC_RETURN &&
            "Unexpected call opcode for a tail call.");
     DAG.getMachineFunction().getFrameInfo().setHasTailCall();
     return DAG.getNode(CallOpc, dl, MVT::Other, Ops);
   }
 
   std::array<EVT, 2> ReturnTypes = {{MVT::Other, MVT::Glue}};
   Chain = DAG.getNode(CallOpc, dl, ReturnTypes, Ops);
   Glue = Chain.getValue(1);
 
   // When performing tail call optimization the callee pops its arguments off
   // the stack. Account for this here so these bytes can be pushed back on in
   // PPCFrameLowering::eliminateCallFramePseudoInstr.
   int BytesCalleePops = (CallConv == CallingConv::Fast &&
                          getTargetMachine().Options.GuaranteedTailCallOpt)
                             ? NumBytes
                             : 0;
 
   Chain = DAG.getCALLSEQ_END(Chain, DAG.getIntPtrConstant(NumBytes, dl, true),
                              DAG.getIntPtrConstant(BytesCalleePops, dl, true),
                              Glue, dl);
   Glue = Chain.getValue(1);
 
   return LowerCallResult(Chain, Glue, CallConv, isVarArg, Ins, dl, DAG, InVals);
 }
 
 SDValue
 PPCTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
                              SmallVectorImpl<SDValue> &InVals) const {
   SelectionDAG &DAG                     = CLI.DAG;
   SDLoc &dl                             = CLI.DL;
   SmallVectorImpl<ISD::OutputArg> &Outs = CLI.Outs;
   SmallVectorImpl<SDValue> &OutVals     = CLI.OutVals;

llvm/test/CodeGen/PowerPC/aix-external-sym-sdnode-lowering.ll

+; RUN: llc -mcpu=pwr4 -mattr=-altivec -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff \
+; RUN: -stop-after=machine-cp < %s | FileCheck \
+; RUN: --check-prefix=32BIT %s
+
+; RUN: llc -mcpu=pwr4 -mattr=-altivec -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff \
+; RUN: -stop-after=machine-cp < %s | FileCheck \
+; RUN: --check-prefix=64BIT %s
+
+define i64 @call_divdi3(i64 %p, i64 %num) {
+entry:
+  %div = sdiv i64 %p, %num
+  ret i64 %div
+}
+
+; 32BIT: BL_NOP <mcsymbol .__divdi3>
+
+define i64 @call_fixunsdfdi(double %p) {
+entry:
+  %conv = fptoui double %p to i64
+  ret i64 %conv
+}
+
+; 32BIT: BL_NOP <mcsymbol .__fixunsdfdi>
+
+define double @call_floatundidf(i64 %p) {
+entry:
+  %conv = uitofp i64 %p to double
+  ret double %conv
+}
+
+; 32BIT: BL_NOP <mcsymbol .__floatundidf>
+
+define float @call_floatundisf(i64 %p) {
+entry:
+  %conv = uitofp i64 %p to float
+  ret float %conv
+}
+
+; 32BIT: BL_NOP <mcsymbol .__floatundisf>
+
+define i64 @call_moddi3(i64 %p, i64 %num) {
+entry:
+  %rem = srem i64 %p, %num
+  ret i64 %rem
+}
+
+; 32BIT: BL_NOP <mcsymbol .__moddi3>
+
+define i64 @call_udivdi3(i64 %p, i64 %q) {
+  %1 = udiv i64 %p, %q
+  ret i64 %1
+}
+
+; 32BIT: BL_NOP <mcsymbol .__udivdi3>
+
+define i64 @call_umoddi3(i64 %p, i64 %num) {
+entry:
+  %rem = urem i64 %p, %num
+  ret i64 %rem
+}
+
+; 32BIT: BL_NOP <mcsymbol .__umoddi3>
+
+define double @call_ceil(double %n) {
+entry:
+  %0 = call double @llvm.ceil.f64(double %n)
+  ret double %0
+}
+
+declare double @llvm.ceil.f64(double)
+
+; 32BIT: BL_NOP <mcsymbol .ceil>
+; 64BIT: BL8_NOP <mcsymbol .ceil>
+
+define double @call_floor(double %n) {
+entry:
+  %0 = call double @llvm.floor.f64(double %n)
+  ret double %0
+}
+
+declare double @llvm.floor.f64(double)
+
+; 32BIT: BL_NOP <mcsymbol .floor>
+; 64BIT: BL8_NOP <mcsymbol .floor>
+
+define void @call_memcpy(i8* %p, i8* %q, i32 %n) {
+entry:
+  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %p, i8* %q, i32 %n, i1 false)
+  ret void
+}
+
+declare void @llvm.memcpy.p0i8.p0i8.i32(i8* nocapture writeonly, i8* nocapture readonly, i32, i1)
+
+; 32BIT: BL_NOP <mcsymbol .memcpy>
+; 64BIT: BL8_NOP <mcsymbol .memcpy>
+
+define void @call_memmove(i8* %p, i8* %q, i32 %n) {
+entry:
+  call void @llvm.memmove.p0i8.p0i8.i32(i8* %p, i8* %q, i32 %n, i1 false)
+  ret void
+}
+
+declare void @llvm.memmove.p0i8.p0i8.i32(i8* nocapture, i8* nocapture readonly, i32, i1)
+
+; 32BIT: BL_NOP <mcsymbol .memmove>
+; 64BIT: BL8_NOP <mcsymbol .memmove>
+
+define void @call_memset(i8* %p, i8 %q, i32 %n) #0 {
+entry:
+  call void @llvm.memset.p0i8.i32(i8* %p, i8 %q, i32 %n, i1 false)
+  ret void
+}
+
+declare void @llvm.memset.p0i8.i32(i8* nocapture, i8, i32, i1)
+
+; 32BIT: BL_NOP <mcsymbol .memset>
+; 64BIT: BL8_NOP <mcsymbol .memset>
+
+define double @call_round(double %n) {
+entry:
+  %0 = call double @llvm.round.f64(double %n)
+  ret double %0
+}
+
+declare double @llvm.round.f64(double)
+
+; 32BIT: BL_NOP <mcsymbol .round>
+; 64BIT: BL8_NOP <mcsymbol .round>

llvm/test/CodeGen/PowerPC/aix-user-defined-memcpy.ll

+; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
+; RUN: -mattr=-altivec -filetype=obj -o %t.o < %s
+
+; RUN: llvm-readobj --syms %t.o | FileCheck --check-prefix=32-SYM %s
+
+; RUN: llvm-readobj --relocs --expand-relocs %t.o | FileCheck \
+; RUN: --check-prefix=32-REL %s
+
+; RUN: not llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff \
+; RUN: -mcpu=pwr4 -mattr=-altivec -filetype=obj < %s 2>&1 | FileCheck \
+; RUN: --check-prefix=64-CHECK %s
+
+; Test verifies:
+; If there exists a user-defined function whose name is the same as the
+; "memcpy" ExternalSymbol's, we pick up the user-defined version, even if this
+; may lead to some undefined behavior.
+
+define dso_local signext i32 @memcpy(i8* %destination, i32 signext %num) {
+entry:
+  ret i32 3
+}
+
+define void @call_memcpy(i8* %p, i8* %q, i32 %n) {
+entry:
+  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %p, i8* %q, i32 %n, i1 false)
+  ret void
+}
+
+declare void @llvm.memcpy.p0i8.p0i8.i32(i8* nocapture writeonly, i8* nocapture readonly, i32, i1)
+
+; TODO: This test should preferably check the symbol table for .o file and
+;       the relocation associated with the call.
+
+; 32-SYM:      Symbol {{[{][[:space:]] *}}Index: [[#Index:]]{{[[:space:]] *}}Name: .memcpy 
+; 32-SYM-NEXT:    Value (RelocatableAddress): 0x0
+; 32-SYM-NEXT:    Section: .text
+; 32-SYM-NEXT:    Type: 0x0
+; 32-SYM-NEXT:    StorageClass: C_EXT (0x2)
+; 32-SYM-NEXT:    NumberOfAuxEntries: 1
+; 32-SYM-NEXT:    CSECT Auxiliary Entry {
+; 32-SYM-NEXT:      Index: 3
+; 32-SYM-NEXT:      ContainingCsectSymbolIndex: 0
+; 32-SYM-NEXT:      ParameterHashIndex: 0x0
+; 32-SYM-NEXT:      TypeChkSectNum: 0x0
+; 32-SYM-NEXT:      SymbolAlignmentLog2: 0
+; 32-SYM-NEXT:      SymbolType: XTY_LD (0x2)
+; 32-SYM-NEXT:      StorageMappingClass: XMC_PR (0x0)
+; 32-SYM-NEXT:      StabInfoIndex: 0x0
+; 32-SYM-NEXT:      StabSectNum: 0x0
+; 32-SYM-NEXT:    }
+; 32-SYM-NEXT:  }
+
+; 32-SYM-NOT: .memcpy
+
+; We are expecting to have the test fail when the support for relocations land.
+; 32-REL-NOT: Relocation{{[[:space:]]}}
+
+; 64-CHECK: LLVM ERROR: 64-bit XCOFF object files are not supported yet.