[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.

730

ditto

lib/Target/PowerPC/PPCSubtarget.cpp

234

You can probably return early here.

240

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

lib/Target/PowerPC/PPCSubtarget.h

289

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

292

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

689–691

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

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

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 GlobalToc = 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())
+      unsigned char GVFlags = Subtarget->classifyGlobalReference(GV);
+      GlobalToc = (GVFlags & PPCII::MO_NLP_FLAG);
+    } 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() ||
+    if (GlobalToc || 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: {
     // Transform %Xd = LDtocL <ga:@sym>, %Xs
else if (MO.isBlockAddress()) {
       MOSymbol = lookUpOrCreateTOCEntry(MOSymbol);
     }
     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)
-        MOSymbol = lookUpOrCreateTOCEntry(MOSymbol);
+      const GlobalValue *GV = MO.getGlobal();
+      MOSymbol = getSymbol(GV);
+      DEBUG(
+        unsigned char GVFlags = Subtarget->classifyGlobalReference(GV);
+        assert((GVFlags & PPCII::MO_NLP_FLAG) &&
+               "LDtocL used on symbol that could be accessed directly is "
+               "invalid. Must match ADDIStocHA."));
     }
 
     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();
+      DEBUG(
+        unsigned char GVFlags = Subtarget->classifyGlobalReference(GV);
+        assert (
+            !(GVFlags & PPCII::MO_NLP_FLAG) &&
+            "Interposable 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 {
     //       LDtocL(GV, ADDIStocHA(%X2, GV))
     // Otherwise we generate:
     //       ADDItocL(ADDIStocHA(%X2, GV), GV)
     // 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())
+    unsigned char GVFlags = PPCSubTarget->classifyGlobalReference(GV);
+    if (GVFlags & PPCII::MO_NLP_FLAG) {
       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: {
     // For medium and large code model, we generate two instructions as
     // described below.  Otherwise we allow SelectCodeCommon to handle this,
     // selecting one of LDtoc, LDtocJTI, LDtocCPT, and LDtocBA.
     CodeModel::Model CModel = TM.getCodeModel();
     if (CModel != CodeModel::Medium && CModel != CodeModel::Large)
       break;
 
     // The first source operand is a TargetGlobalAddress or a TargetJumpTable.
-    // If it is an externally defined symbol, a symbol with common linkage,
-    // a non-local function address, or a jump table address, or if we are
-    // generating code for large code model, we generate:
+    // If it must be toc-referenced according to PPCSubTarget, we generate:
     //   LDtocL(<ga:@sym>, ADDIStocHA(%X2, <ga:@sym>))
     // Otherwise we generate:
     //   ADDItocL(ADDIStocHA(%X2, <ga:@sym>), <ga:@sym>)
     SDValue GA = N->getOperand(0);
     SDValue TOCbase = N->getOperand(1);
     SDNode *Tmp = CurDAG->getMachineNode(PPC::ADDIStocHA, dl, MVT::i64,
                                          TOCbase, GA);
 
     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())
+      const GlobalValue *GV = G->getGlobal();
+      unsigned char GVFlags = PPCSubTarget->classifyGlobalReference(GV);
+      if (GVFlags & PPCII::MO_NLP_FLAG) {
         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: {
     // Generate a PIC-safe GOT reference.
     assert(!PPCSubTarget->isPPC64() && PPCSubTarget->isSVR4ABI() &&
       "PPCISD::PPC32_PICGOT is only supported for 32-bit SVR4");

lib/Target/PowerPC/PPCSubtarget.h

public:
 
   void overrideSchedPolicy(MachineSchedPolicy &Policy,
                            MachineInstr *begin,
                            MachineInstr *end,
                            unsigned NumRegionInstrs) const override;
   bool useAA() const override;
 
   bool enableSubRegLiveness() const override;
+
+  /// classifyGlobalReference - Classify a global variable reference for the
+  /// current subtarget accourding to how we should reference it.
+  unsigned char classifyGlobalReference(const GlobalValue *GV) const;
 };
 } // End llvm namespace
 
 #endif

lib/Target/PowerPC/PPCSubtarget.cpp

bool PPCSubtarget::useAA() const {
   // Use AA during code generation for the embedded cores.
   return needsAggressiveScheduling(DarwinDirective);
 }
 
 bool PPCSubtarget::enableSubRegLiveness() const {
   return UseSubRegLiveness;
 }
 
+unsigned char PPCSubtarget::classifyGlobalReference(
+    const GlobalValue *GV) const {
+  // Note that currently we don't generate non-pic references.
+  // If a caller wants that, this will have to be updated.
+
+  // Large code model always uses the TOC even for local symbols.
+  if (TM.getCodeModel() == CodeModel::Large)
+    return PPCII::MO_PIC_FLAG | PPCII::MO_NLP_FLAG;
+
+  unsigned char flags = PPCII::MO_PIC_FLAG;
+
+  // Only if the relocation mode is PIC do we have to worry about
+  // interposition. In all other cases we can use a slightly looser standard to
+  // decide how to access the symbol.
+  if (TM.getRelocationModel() == Reloc::PIC_) {
+    // If it's local, or it's non-default, it can't be interposed.
+    if (!GV->hasLocalLinkage() &&
+        GV->hasDefaultVisibility()) {
+      flags |= PPCII::MO_NLP_FLAG;
+    }
+    return flags;
+  }
+
+  if (GV->isStrongDefinitionForLinker())
+    return flags;
+  return flags | PPCII::MO_NLP_FLAG;
+}
+
 bool PPCSubtarget::isELFv2ABI() const { return TM.isELFv2ABI(); }
 bool PPCSubtarget::isPPC64() const { return TM.isPPC64(); }

test/CodeGen/PowerPC/mcm-13.ll

+; RUN: llc -mcpu=pwr7 -O0 -code-model=medium <%s | FileCheck %s
+; RUN: llc -mcpu=pwr7 -O0 -code-model=large <%s | FileCheck %s
+
+; Test correct code generation for medium and large code model
+; for loading and storing a weak variable
+
+target datalayout = "E-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-f128:128:128-v128:128:128-n32:64"
+target triple = "powerpc64-unknown-linux-gnu"
+
+@wi = weak global i32 0, align 4
+
+define signext i32 @test_weak() nounwind {
+entry:
+  %0 = load i32, i32* @wi, align 4
+  %inc = add nsw i32 %0, 1
+  store i32 %inc, i32* @wi, align 4
+  ret i32 %0
+}
+
+; CHECK-LABEL: test_weak:
+; CHECK: addis [[REG1:[0-9]+]], 2, .LC[[TOCNUM:[0-9]+]]@toc@ha
+; CHECK: ld [[REG2:[0-9]+]], .LC[[TOCNUM]]@toc@l([[REG1]])
+; CHECK: lwz {{[0-9]+}}, 0([[REG2]])
+; CHECK: stw {{[0-9]+}}, 0([[REG2]])
+; CHECK: .section .toc
+; CHECK: .LC[[TOCNUM]]:
+; CHECK: .tc {{[a-z0-9A-Z_.]+}}[TC],{{[a-z0-9A-Z_.]+}}