[NFC][PowerPC]Clean up PPCAsmPrinter for TOC related pseudo opcode

[Xiangling_L]

Summary

Add a helper function getMCSymbolForTOCPseudoMO to clean up PPCAsmPrinter a little bit.

[jsji]

LGTM. Some nit comments.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

744

MOSymbol used only once here, maybe inline the call directly?

763

large code model only for CPI?

776

Are these clang formatted? Formatting looks weird to me.

799

large code model only for CPI?

837

MOSymbol used only once here, maybe inline the call directly?

[Xiangling_L]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

776

Yes, they are.

[Xiangling_L]

Address commments & add const for lookUpOrCreateTOCEntry & TOC MapVector<const MCSymbol *, MCSymbol *>

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

517

Has a switch on getType been considered?

687

Is it also possible to use * const?

739

There's an extra space in the indentation here.

762

Please don't remove the Oxford comma before the "or".

773

Can this be const?

776

This does not seem to be an NFC change.

799

Comma before "then".

800

Comma after "Otherwise".

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

838

Missing the invocation of assert here?

1378–1379

Has a range-based for loop been considered?

[Xiangling_L]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

517

Sure.

776

Since Machine Operand for this TOC pseudo opcode can only be MO.isGlobal() || MO.isCPI() || MO.isJTI() || MO.isBlockAddress(), so the line`(MO.isCPI() && TM.getCodeModel() == CodeModel::Large) will have the same logic with or without MO.isCPI(). But since people @jsji have some slight preference to make it clear, I add MO.isCPI()`. Please let me know if you still have any concerns about this change.

838

Thank you for mentioning this mistake.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

776

I do not believe that the statement is sufficiently supported in this context except if there is:

assert(GlobalToc || !MO.isGlobal());

[Xiangling_L]

Address 2nd round comments

[Xiangling_L]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

776

I think I kinda know your concern, do you mean without my changes, for query if (GlobalToc || MO.isJTI() || MO.isBlockAddress() || TM.getCodeModel() == CodeModel::Large), there is the possibility that GlobalToc is false, but the code model is large, under which we still generate TOCEntry for global MO? If my understanding is correct, then I think we don't need to worry about this situation, because according to the definition of isGVIndirectSymbol:

bool PPCSubtarget::isGVIndirectSymbol(const GlobalValue *GV) const {
  // Large code model always uses the TOC even for local symbols.
  if (TM.getCodeModel() == CodeModel::Large)
    return true;
  if (TM.shouldAssumeDSOLocal(*GV->getParent(), GV))
    return false;
  return true;
}

When it's large code model, and MO.isGlobal() == true, GlobalToc is definitely true, so there is no situation that when MO.isGlobal() &&GlobalToc == false, but the code model is not large.

[Xiangling_L]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

776

typo: not large --> large

[hubert.reinterpretcast]

LGTM with some straightforward changes (can be fixed on the commit).

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

687

My fault on the previous comment; please adjust the space: const MCSymbol *const MOSymbol.

776

Okay, thanks.

1379

I prefer to use meaningful names:

for (const auto &TOCMapPair : TOC) {
  const MCSymbol *const TOCEntryTarget = TOCMapPair.first;
  MCSymbol *const TOCEntryLabel = TOCMapPair.second;

  OutStreamer->EmitLabel(TOCEntryLabel);
  if (isPPC64) {
    TS.emitTCEntry(*TOCEntryTarget);
  } else {
    OutStreamer->EmitValueToAlignment(4);
    OutStreamer->EmitSymbolValue(TOCEntryTarget, 4)
  }
}

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

515

s/the machine operand/a machine operand for a TOC pseudo-machine instruction/;

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

 using namespace llvm;
 
 #define DEBUG_TYPE "asmprinter"
 
 namespace {
 
 class PPCAsmPrinter : public AsmPrinter {
 protected:
-  MapVector<MCSymbol *, MCSymbol *> TOC;
+  MapVector<const MCSymbol *, MCSymbol *> TOC;
   const PPCSubtarget *Subtarget;
   StackMaps SM;
 
 public:
   explicit PPCAsmPrinter(TargetMachine &TM,
                          std::unique_ptr<MCStreamer> Streamer)
       : AsmPrinter(TM, std::move(Streamer)), SM(*this) {}
 
   StringRef getPassName() const override { return "PowerPC Assembly Printer"; }
 
-  MCSymbol *lookUpOrCreateTOCEntry(MCSymbol *Sym);
+  MCSymbol *lookUpOrCreateTOCEntry(const MCSymbol *Sym);
 
   bool doInitialization(Module &M) override {
     if (!TOC.empty())
       TOC.clear();
     return AsmPrinter::doInitialization(M);
   }
 
   void EmitInstruction(const MachineInstr *MI) override;
bool PPCAsmPrinter::PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
   printOperand(MI, OpNo, O);
   O << ")";
   return false;
 }
 
 /// lookUpOrCreateTOCEntry -- Given a symbol, look up whether a TOC entry
 /// exists for it.  If not, create one.  Then return a symbol that references
 /// the TOC entry.
-MCSymbol *PPCAsmPrinter::lookUpOrCreateTOCEntry(MCSymbol *Sym) {
+MCSymbol *PPCAsmPrinter::lookUpOrCreateTOCEntry(const MCSymbol *Sym) {
   MCSymbol *&TOCEntry = TOC[Sym];
   if (!TOCEntry)
     TOCEntry = createTempSymbol("C");
   return TOCEntry;
 }
 
 void PPCAsmPrinter::EmitEndOfAsmFile(Module &M) {
   emitStackMaps(SM);
void PPCAsmPrinter::EmitTlsCall(const MachineInstr *MI,
   const MCExpr *SymVar = MCSymbolRefExpr::create(MOSymbol, VK, OutContext);
   EmitToStreamer(*OutStreamer,
                  MCInstBuilder(Subtarget->isPPC64() ?
                                PPC::BL8_NOP_TLS : PPC::BL_TLS)
                  .addExpr(TlsRef)
                  .addExpr(SymVar));
 }
 
+/// Map the machine operand to its corresponding MCSymbol.
+static MCSymbol *getMCSymbolForTOCPseudoMO(const MachineOperand &MO, AsmPrinter &AP) {
+  switch(MO.getType()) {
+  case MachineOperand::MO_GlobalAddress:
+    return AP.getSymbol(MO.getGlobal());
+  case MachineOperand::MO_ConstantPoolIndex:
+    return AP.GetCPISymbol(MO.getIndex());
+  case MachineOperand::MO_JumpTableIndex:
+    return AP.GetJTISymbol(MO.getIndex());
+  case MachineOperand::MO_BlockAddress:
+    return AP.GetBlockAddressSymbol(MO.getBlockAddress());
+  default:
+    llvm_unreachable("Unexpected operand type to get symbol.");
+  }
+}
+
 /// EmitInstruction -- Print out a single PowerPC MI in Darwin syntax to
 /// the current output stream.
 ///
 void PPCAsmPrinter::EmitInstruction(const MachineInstr *MI) {
   MCInst TmpInst;
   const bool IsDarwin = TM.getTargetTriple().isOSDarwin();
   const Module *M = MF->getFunction().getParent();
   PICLevel::Level PL = M->getPICLevel();
case PPC::LWZtoc: {
     // Change the opcode to LWZ.
     TmpInst.setOpcode(PPC::LWZ);
 
     const MachineOperand &MO = MI->getOperand(1);
     assert((MO.isGlobal() || MO.isCPI() || MO.isJTI() || MO.isBlockAddress()) &&
            "Unexpected operand type for LWZtoc pseudo.");
 
     // Map the operand to its corresponding MCSymbol.
-    MCSymbol *MOSymbol = nullptr;
-    if (MO.isGlobal())
-      MOSymbol = getSymbol(MO.getGlobal());
-    else if (MO.isCPI())
-      MOSymbol = GetCPISymbol(MO.getIndex());
-    else if (MO.isJTI())
-      MOSymbol = GetJTISymbol(MO.getIndex());
-    else if (MO.isBlockAddress())
-      MOSymbol = GetBlockAddressSymbol(MO.getBlockAddress());
-
+    const MCSymbol* const MOSymbol = getMCSymbolForTOCPseudoMO(MO, *this);
     const bool IsAIX = TM.getTargetTriple().isOSAIX();
 
     // Create a reference to the GOT entry for the symbol. The GOT entry will be
     // synthesized later.
     if (PL == PICLevel::SmallPIC && !IsAIX) {
       const MCExpr *Exp =
         MCSymbolRefExpr::create(MOSymbol, MCSymbolRefExpr::VK_GOT,
                                 OutContext);
#endif
   }
   case PPC::LDtocJTI:
   case PPC::LDtocCPT:
   case PPC::LDtocBA:
   case PPC::LDtoc: {
     // Transform %x3 = LDtoc @min1, %x2
     LowerPPCMachineInstrToMCInst(MI, TmpInst, *this, IsDarwin);
 
-    // Change the opcode to LD, and the global address operand to be a
-    // reference to the TOC entry we will synthesize later.
+    // Change the opcode to LD.
     TmpInst.setOpcode(PPC::LD);
+
     const MachineOperand &MO = MI->getOperand(1);
+    assert((MO.isGlobal() || MO.isCPI() || MO.isJTI() || MO.isBlockAddress()) &&
+           "Invalid operand!");
 
-    // Map symbol -> label of TOC entry
-    assert(MO.isGlobal() || MO.isCPI() || MO.isJTI() || MO.isBlockAddress());
-    MCSymbol *MOSymbol = nullptr;
-    if (MO.isGlobal())
-      MOSymbol = getSymbol(MO.getGlobal());
-    else if (MO.isCPI())
-      MOSymbol = GetCPISymbol(MO.getIndex());
-    else if (MO.isJTI())
-      MOSymbol = GetJTISymbol(MO.getIndex());
-    else if (MO.isBlockAddress())
-      MOSymbol = GetBlockAddressSymbol(MO.getBlockAddress());
-
-    MCSymbol *TOCEntry = lookUpOrCreateTOCEntry(MOSymbol);
+    // Map the machine operand to its corresponding MCSymbol, then map the
+    // global address operand to be a reference to the TOC entry we will
+    // synthesize later.
+    MCSymbol *TOCEntry =
+        lookUpOrCreateTOCEntry(getMCSymbolForTOCPseudoMO(MO, *this));
 
     const MCExpr *Exp =
       MCSymbolRefExpr::create(TOCEntry, MCSymbolRefExpr::VK_PPC_TOC,
                               OutContext);
     TmpInst.getOperand(1) = MCOperand::createExpr(Exp);
     EmitToStreamer(*OutStreamer, TmpInst);
     return;
   }
 
   case PPC::ADDIStocHA8: {
     // Transform %xd = ADDIStocHA8 %x2, @sym
     LowerPPCMachineInstrToMCInst(MI, TmpInst, *this, IsDarwin);
 
-    // Change the opcode to ADDIS8.  If the global address is external, has
-    // common linkage, is a non-local function address, or is a jump table
-    // address, then generate a TOC entry and reference that.  Otherwise
-    // reference the symbol directly.
+    // Change the opcode to ADDIS8. If the global address is the address of
+    // an external symbol, is a jump table address, is a block address, or is a
+    // constant pool index with large code model enabled, 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()) &&
+    assert((MO.isGlobal() || MO.isCPI() || MO.isJTI() || MO.isBlockAddress()) &&
            "Invalid operand for ADDIStocHA8!");
-    MCSymbol *MOSymbol = nullptr;
-    bool GlobalToc = false;
 
-    if (MO.isGlobal()) {
-      const GlobalValue *GV = MO.getGlobal();
-      MOSymbol = getSymbol(GV);
-      GlobalToc = Subtarget->isGVIndirectSymbol(GV);
-    } else if (MO.isCPI()) {
-      MOSymbol = GetCPISymbol(MO.getIndex());
-    } else if (MO.isJTI()) {
-      MOSymbol = GetJTISymbol(MO.getIndex());
-    } else if (MO.isBlockAddress()) {
-      MOSymbol = GetBlockAddressSymbol(MO.getBlockAddress());
-    }
+    const MCSymbol *MOSymbol = getMCSymbolForTOCPseudoMO(MO, *this);
 
+    const bool GlobalToc = 
+  MO.isGlobal() && Subtarget->isGVIndirectSymbol(MO.getGlobal());
     if (GlobalToc || MO.isJTI() || MO.isBlockAddress() ||
-        TM.getCodeModel() == CodeModel::Large)
+  (MO.isCPI() && TM.getCodeModel() == CodeModel::Large))
       MOSymbol = lookUpOrCreateTOCEntry(MOSymbol);
 
     const MCExpr *Exp =
       MCSymbolRefExpr::create(MOSymbol, MCSymbolRefExpr::VK_PPC_TOC_HA,
                               OutContext);
 
     if (!MO.isJTI() && MO.getOffset())
       Exp = MCBinaryExpr::createAdd(Exp,
                                     MCConstantExpr::create(MO.getOffset(),
                                                            OutContext),
                                     OutContext);
 
     TmpInst.getOperand(2) = MCOperand::createExpr(Exp);
     EmitToStreamer(*OutStreamer, TmpInst);
     return;
   }
   case PPC::LDtocL: {
     // Transform %xd = LDtocL @sym, %xs
     LowerPPCMachineInstrToMCInst(MI, TmpInst, *this, IsDarwin);
 
-    // Change the opcode to LD.  If the global address is external, has
-    // common linkage, or is a jump table address, then reference the
-    // associated TOC entry.  Otherwise reference the symbol directly.
+    // Change the opcode to LD. If the global address is the address of
+    // an external symbol, is a jump table address, is a block address, or is
+    // a constant pool index with large code model enabled, then generate a
+    // TOC entry and reference that. Otherwise, reference the symbol directly.
     TmpInst.setOpcode(PPC::LD);
+
     const MachineOperand &MO = MI->getOperand(1);
     assert((MO.isGlobal() || MO.isCPI() || MO.isJTI() ||
             MO.isBlockAddress()) &&
            "Invalid operand for LDtocL!");
-    MCSymbol *MOSymbol = nullptr;
 
-    if (MO.isJTI())
-      MOSymbol = lookUpOrCreateTOCEntry(GetJTISymbol(MO.getIndex()));
-    else if (MO.isBlockAddress()) {
-      MOSymbol = GetBlockAddressSymbol(MO.getBlockAddress());
-      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 *GV = MO.getGlobal();
-      MOSymbol = getSymbol(GV);
-      LLVM_DEBUG(
-          assert((Subtarget->isGVIndirectSymbol(GV)) &&
-                 "LDtocL used on symbol that could be accessed directly is "
-                 "invalid. Must match ADDIStocHA8."));
+    LLVM_DEBUG(assert(
+        (!MO.isGlobal() || Subtarget->isGVIndirectSymbol(MO.getGlobal())) &&
+
+    const MCSymbol *MOSymbol = getMCSymbolForTOCPseudoMO(MO, *this);
+
+    if (!MO.isCPI() || TM.getCodeModel() == CodeModel::Large)
       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, @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
+    // 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;
+    assert((MO.isGlobal() || MO.isCPI()) && "Invalid operand for ADDItocL.");
 
-    if (MO.isGlobal()) {
-      const GlobalValue *GV = MO.getGlobal();
-      LLVM_DEBUG(assert(!(Subtarget->isGVIndirectSymbol(GV)) &&
-                        "Interposable definitions must use indirect access."));
-      MOSymbol = getSymbol(GV);
-    } else if (MO.isCPI()) {
-      MOSymbol = GetCPISymbol(MO.getIndex());
-    }
+    LLVM_DEBUG(
+        assert(!(MO.isGlobal() && Subtarget->isGVIndirectSymbol(MO.getGlobal())) &&
 
     const MCExpr *Exp =
-      MCSymbolRefExpr::create(MOSymbol, MCSymbolRefExpr::VK_PPC_TOC_LO,
-                              OutContext);
+        MCSymbolRefExpr::create(getMCSymbolForTOCPseudoMO(MO, *this),
+                                MCSymbolRefExpr::VK_PPC_TOC_LO, OutContext);
     TmpInst.getOperand(2) = MCOperand::createExpr(Exp);
     EmitToStreamer(*OutStreamer, TmpInst);
     return;
   }
   case PPC::ADDISgotTprelHA: {
     // Transform: %xd = ADDISgotTprelHA %x2, @sym
     // Into:      %xd = ADDIS8 %x2, sym@got@tlsgd@ha
     assert(Subtarget->isPPC64() && "Not supported for 32-bit PowerPC");
if (!TOC.empty()) {
 
     if (isPPC64)
       Section = OutStreamer->getContext().getELFSection(
           ".toc", ELF::SHT_PROGBITS, ELF::SHF_WRITE | ELF::SHF_ALLOC);
         else
           Section = OutStreamer->getContext().getELFSection(
               ".got2", ELF::SHT_PROGBITS, ELF::SHF_WRITE | ELF::SHF_ALLOC);
     OutStreamer->SwitchSection(Section);
 
-    for (MapVector<MCSymbol*, MCSymbol*>::iterator I = TOC.begin(),
-         E = TOC.end(); I != E; ++I) {
-      OutStreamer->EmitLabel(I->second);
-      MCSymbol *S = I->first;
+    for (const auto &I : TOC) {
+      OutStreamer->EmitLabel(I.second);
+      const MCSymbol *S = I.first;
       if (isPPC64) {
         TS.emitTCEntry(*S);
       } else {
         OutStreamer->EmitValueToAlignment(4);
         OutStreamer->EmitSymbolValue(S, 4);
       }
     }
   }