[PPC] Fix indirect access for weak symbols.

[iteratee]

Summary

Weak non-function symbols were being accessed directly, which is incorrect, as
the chosen representative of the weak symbol may not live with the code
in question. Always indirect the access through the TOC instead.

[wschmidt]

Thanks, Kyle. This LGTM. That's a much better interface to use (and one that didn't exist when this code was first written). Thanks for taking care of this!

lib/Target/PowerPC/PPCAsmPrinter.cpp

678

Interesting. People often tell me not to use braces around a single statement, but as I check the coding standards for the projects, this is not mentioned. I prefer your style anyway. Just a heads-up to other reviewers that I'm not gonna take it anymore! ;)

[rnk]

This should really be checking visibility, not linkage. See the implementations of ClassifyGlobalReference for AArch64 and X86.

I believe the logic should be: if this is a local definition or (a strong definition that has hidden or protected visibility), then we can skip the TOC reference. Otherwise, we must use the TOC because the symbol could be interposed.

[iteratee]

As Eric and Reid mentioned, this should be based on visibility. I created a method in PPCSubtarget to factor this out and made it use visibility.

This caused some tests to fail because it allowed non-toc access to some local symbols in the tests. They've been updated to match.

[iteratee]

There is currently a bug in gold that prevents using non-toc accesses even when the abi would allow it.

Document that bug in the comments, and work around it.
The tests revert in this case as well.

[iteratee]

Relax symbol classification when relocation model is not PIC

Per a discussion with rnk:
If the relocation model is not PIC, then it is safe to use a local definition for non-weak symbols.
Otherwise they may be interposed. This reverts all the tests, as none of them were using PIC access.

[iteratee]

Revert unneeded fix for test and fix typo.

[rnk]

lib/Target/PowerPC/PPCAsmPrinter.cpp

680

gv_flags isn't LLVM style, GVFlags would be.

735

ditto

lib/Target/PowerPC/PPCSubtarget.cpp

234 ↗

(On Diff #40586)

You can probably return early here.

240 ↗

(On Diff #40586)

You don't need this return, falling through to return flags would be fine

lib/Target/PowerPC/PPCSubtarget.h

293 ↗

(On Diff #40586)

The case of the name of the method doesn't match. You can just delete it and the Doxygen will probably come out fine.

296 ↗

(On Diff #40586)

Nobody calls this with forcePIC=false, right? Is the intention that this will change in the future? For now, I'd just remove the parameter and check from the classification function and put in a comment about what we'd have to return to form a direct access.

You can also use a default argument here instead of an overload:

unsigned char classifyGlobalReference(const GlobalValue *GV, bool forcePIC = false) const;

[iteratee]

Fixed comments by rnk

[iteratee]

Replying to comments.

lib/Target/PowerPC/PPCAsmPrinter.cpp

678

I changed this one because it bothers me to see an if with braces when the else doesn't have one or vice versa.

I'm OK with small statements not having any, but mixing them seemed ugly to me.

[echristo]

One inline nit. Otherwise rnk has been handling it :)

lib/Target/PowerPC/PPCAsmPrinter.cpp

687–689

I might buy the argument above for single line braces in context of lots of if/else chains. This one isn't though :)

lib/Target/PowerPC/PPCSubtarget.cpp

213–214 ↗

(On Diff #40730)

This is really over 80-col? Sad.

[iteratee]

Fixed minor nit.

[rnk]

lgtm

[echristo]

Hi Kyle,

Was going to commit this but noticed the testcase changes never made it into the diff?

-eric

[iteratee]

lib/Target/PowerPC/PPCSubtarget.cpp

213–214 ↗

(On Diff #40730)

Blame const.

[iteratee]

Test case as requested.

[echristo]

Testcase LGTM, Thanks!

-eric

[echristo]

I've committed this thusly:

M lib/Target/PowerPC/PPCISelDAGToDAG.cpp
M lib/Target/PowerPC/PPCFastISel.cpp
M lib/Target/PowerPC/PPCAsmPrinter.cpp
M lib/Target/PowerPC/PPCSubtarget.cpp
M lib/Target/PowerPC/PPCSubtarget.h
A test/CodeGen/PowerPC/mcm-13.ll
r253708 = 1d60d1ecfc3b51bbecc7bf08f59d372397e7772b
(refs/remotes/origin/master)

-eric

lib/Target/PowerPC/PPCAsmPrinter.cpp

case PPC::ADDIStocHA: {
     // address, then generate a TOC entry and reference that.  Otherwise
     // reference the symbol directly.
     TmpInst.setOpcode(PPC::ADDIS8);
     const MachineOperand &MO = MI->getOperand(2);
     assert((MO.isGlobal() || MO.isCPI() || MO.isJTI() ||
             MO.isBlockAddress()) &&
            "Invalid operand for ADDIStocHA!");
     MCSymbol *MOSymbol = nullptr;
-    bool IsExternal = false;
-    bool IsNonLocalFunction = false;
-    bool IsCommon = false;
-    bool IsAvailExt = false;
+    bool IsWeak = false;
 
     if (MO.isGlobal()) {
       const GlobalValue *GV = MO.getGlobal();
       MOSymbol = getSymbol(GV);
-      IsExternal = GV->isDeclaration();
-      IsCommon = GV->hasCommonLinkage();
-      IsNonLocalFunction = GV->getType()->getElementType()->isFunctionTy() &&
-        !GV->isStrongDefinitionForLinker();
-      IsAvailExt = GV->hasAvailableExternallyLinkage();
-    } else if (MO.isCPI())
+      IsWeak = !GV->isStrongDefinitionForLinker();
+    } else if (MO.isCPI()) {
       MOSymbol = GetCPISymbol(MO.getIndex());
-    else if (MO.isJTI())
+    } else if (MO.isJTI()) {
       MOSymbol = GetJTISymbol(MO.getIndex());
-    else if (MO.isBlockAddress())
+    } else if (MO.isBlockAddress()) {
       MOSymbol = GetBlockAddressSymbol(MO.getBlockAddress());
+    }
 
-    if (IsExternal || IsNonLocalFunction || IsCommon || IsAvailExt ||
-        MO.isJTI() || MO.isBlockAddress() ||
-        TM.getCodeModel() == CodeModel::Large)
+    if (IsWeak || MO.isJTI() || MO.isBlockAddress() ||
+        TM.getCodeModel() == CodeModel::Large) {
       MOSymbol = lookUpOrCreateTOCEntry(MOSymbol);
+    }
 
     const MCExpr *Exp =
       MCSymbolRefExpr::create(MOSymbol, MCSymbolRefExpr::VK_PPC_TOC_HA,
                               OutContext);
     TmpInst.getOperand(2) = MCOperand::createExpr(Exp);
     EmitToStreamer(*OutStreamer, TmpInst);
     return;
   }
case PPC::LDtocL: {
     else if (MO.isCPI()) {
       MOSymbol = GetCPISymbol(MO.getIndex());
       if (TM.getCodeModel() == CodeModel::Large)
         MOSymbol = lookUpOrCreateTOCEntry(MOSymbol);
     }
     else if (MO.isGlobal()) {
       const GlobalValue *GValue = MO.getGlobal();
       MOSymbol = getSymbol(GValue);
-      if (GValue->getType()->getElementType()->isFunctionTy() ||
-          GValue->isDeclaration() || GValue->hasCommonLinkage() ||
-          GValue->hasAvailableExternallyLinkage() ||
-          TM.getCodeModel() == CodeModel::Large)
+      assert((!GValue->isStrongDefinitionForLinker() ||
+              TM.getCodeModel() == CodeModel::Large) &&
+             "Strong definitions must be accessed directly to match ADDIStocHA.");
         MOSymbol = lookUpOrCreateTOCEntry(MOSymbol);
     }
 
     const MCExpr *Exp =
       MCSymbolRefExpr::create(MOSymbol, MCSymbolRefExpr::VK_PPC_TOC_LO,
                               OutContext);
     TmpInst.getOperand(1) = MCOperand::createExpr(Exp);
     EmitToStreamer(*OutStreamer, TmpInst);
     return;
   }
   case PPC::ADDItocL: {
     // Transform %Xd = ADDItocL %Xs, <ga:@sym>
     LowerPPCMachineInstrToMCInst(MI, TmpInst, *this, isDarwin);
 
     // Change the opcode to ADDI8.  If the global address is external, then
     // generate a TOC entry and reference that.  Otherwise reference the
     // symbol directly.
     TmpInst.setOpcode(PPC::ADDI8);
     const MachineOperand &MO = MI->getOperand(2);
     assert((MO.isGlobal() || MO.isCPI()) && "Invalid operand for ADDItocL");
     MCSymbol *MOSymbol = nullptr;
-    bool IsExternal = false;
-    bool IsNonLocalFunction = false;
 
     if (MO.isGlobal()) {
       const GlobalValue *GV = MO.getGlobal();
+      assert ((GV.isStrongDefinitionForLinker() ||
+               TM.getCodeModel() != CodeModel::Large) &&
+              "Non-strong definitions must use indirect access.");
       MOSymbol = getSymbol(GV);
-      IsExternal = GV->isDeclaration();
-      IsNonLocalFunction = GV->getType()->getElementType()->isFunctionTy() &&
-        !GV->isStrongDefinitionForLinker();
-    } else if (MO.isCPI())
+    } else if (MO.isCPI()) {
       MOSymbol = GetCPISymbol(MO.getIndex());
-
-    if (IsNonLocalFunction || IsExternal ||
-        TM.getCodeModel() == CodeModel::Large)
-      MOSymbol = lookUpOrCreateTOCEntry(MOSymbol);
+    }
 
     const MCExpr *Exp =
       MCSymbolRefExpr::create(MOSymbol, MCSymbolRefExpr::VK_PPC_TOC_LO,
                               OutContext);
     TmpInst.getOperand(2) = MCOperand::createExpr(Exp);
     EmitToStreamer(*OutStreamer, TmpInst);
     return;
   }

lib/Target/PowerPC/PPCFastISel.cpp

else {
     // Either way, start with the ADDIStocHA:
     unsigned HighPartReg = createResultReg(RC);
     BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(PPC::ADDIStocHA),
             HighPartReg).addReg(PPC::X2).addGlobalAddress(GV);
 
     // If/when switches are implemented, jump tables should be handled
     // on the "if" path here.
     if (CModel == CodeModel::Large ||
-        (GV->getType()->getElementType()->isFunctionTy() &&
-         !GV->isStrongDefinitionForLinker()) ||
-        GV->isDeclaration() || GV->hasCommonLinkage() ||
-        GV->hasAvailableExternallyLinkage())
+        !GV->isStrongDefinitionForLinker()) {
       BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(PPC::LDtocL),
               DestReg).addGlobalAddress(GV).addReg(HighPartReg);
-    else
+    } else {
       // Otherwise generate the ADDItocL.
       BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(PPC::ADDItocL),
               DestReg).addReg(HighPartReg).addGlobalAddress(GV);
     }
+  }
 
   return DestReg;
 }
 
 // Materialize a 32-bit integer constant into a register, and return
 // the register number (or zero if we failed to handle it).
 unsigned PPCFastISel::PPCMaterialize32BitInt(int64_t Imm,
                                              const TargetRegisterClass *RC) {

lib/Target/PowerPC/PPCISelDAGToDAG.cpp

case PPCISD::TOC_ENTRY: {
 
     if (isa<JumpTableSDNode>(GA) || isa<BlockAddressSDNode>(GA) ||
         CModel == CodeModel::Large)
       return transferMemOperands(N, CurDAG->getMachineNode(PPC::LDtocL, dl,
                                       MVT::i64, GA, SDValue(Tmp, 0)));
 
     if (GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(GA)) {
       const GlobalValue *GValue = G->getGlobal();
-      if ((GValue->getType()->getElementType()->isFunctionTy() &&
-           !GValue->isStrongDefinitionForLinker()) ||
-          GValue->isDeclaration() || GValue->hasCommonLinkage() ||
-          GValue->hasAvailableExternallyLinkage())
+      if (!GValue->isStrongDefinitionForLinker())
         return transferMemOperands(N, CurDAG->getMachineNode(PPC::LDtocL, dl,
                                         MVT::i64, GA, SDValue(Tmp, 0)));
     }
 
     return CurDAG->getMachineNode(PPC::ADDItocL, dl, MVT::i64,
                                   SDValue(Tmp, 0), GA);
   }
   case PPCISD::PPC32_PICGOT: {