[NFC][PPC] Refactor TOC representation to allow several entries for the same symbol

[NeHuang]

Summary

We currently represent TOC entries by an MCSymbol. This is not
enough in some situations. For example, when accessing an
initialized TLS variable v on AIX using the general dynamic model, we
need to generate the two following entries for v:

.tc .v[TC],v@m
.tc v[TC],v

One is for the region handle (with the @m relocation), the other is for
the variable offset.
This refactoring allows storing several entries for the same symbol
with different VariantKind in the TOC. If the VariantKind is not
specified, we default to VK_None.

The AIX TLS implementation using this refactoring to generate the two
entries will be posted in a subsequent patch.

[amyk]

llvm/lib/MC/MCExpr.cpp

225 ↗

(On Diff #322397)

nit: Address the clang format.

Also, sorry, this might be a silly question. What is the difference between VK_None and VK_Empty?

[bsaleil]

llvm/lib/MC/MCExpr.cpp

225 ↗

(On Diff #322397)

You can see in MCExpr.h that VK_Empty is used to represent an empty key in a DenseMap. If I understand correctly how DenseMap works, when inserting a value, if there is a collision, it looks for a slot with an empty key to put the value. So if we use VK_None as the empty key, this prevents us from actually inserting the VK_None value in the table, because it may be overwritten by a later insertion.

[sfertile]

llvm/include/llvm/MC/MCExpr.h

421 ↗

(On Diff #322397)

I'm not sure why we need this, or even why this works ... IIUC MapVector creates a dense map from the key type to the index of the value. Shouldn't we be creating a DenseMapInfo of the key type which is now a std::pair<const MCSymbol *, MCSymbolRefExpr::VariantKind>?

[bsaleil]

llvm/include/llvm/MC/MCExpr.h

421 ↗

(On Diff #322397)

There is a generic implementation of DenseMapInfo<std::pair<T, U>> in DenseMapInfo.h which relies on DenseMapInfo<T> and DenseMapInfo<U>.

[sfertile]

llvm/include/llvm/MC/MCExpr.h

421 ↗

(On Diff #322397)

Thanks Baptiste. I'm a little hesitant to add a new value to the variant kind enum that only makes sense as a special value in a dense map. Would you consider creating the DenseMapInfo for the std::pair<const MCSymbol *, MCSymbolRefExpr::VariantKind> and not using the generic implementation of DenseMapInfo<std::pair<T, U>>? I know it is ugly, but it will be relegated to PPCAsmPrinter.cpp and we can avoid adding a new value to the VariantKinds.

[bsaleil]

Specialize DenseMapInfo directly with the PPC TOC key type instead of VariantKind.

[bsaleil]

llvm/include/llvm/MC/MCExpr.h

421 ↗

(On Diff #322397)

Sure, I just updated the patch to make that change.

[sfertile]

Thanks for the update. LGTM.

[jankratochvil]

Unfortunately it fails to build with GCC which LLVM still tries to stay compatible with:
http://lab.llvm.org:8014/#/builders/16/builds/2211
gcc-11.0.0-0.19.fc34.x86_64
gcc-10.2.1-9.fc33.x86_64
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:86:8: error: explicit specialization of 'template<class T> struct llvm::DenseMapInfo' outside its namespace must use a nested-name-specifier [-fpermissive]

86 | struct DenseMapInfo<std::pair<const MCSymbol *, MCSymbolRefExpr::VariantKind>> {
   |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I have filed it to an existing GCC bugreport: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92598

[dyung]

In D96346#2567215, @jankratochvil wrote:

Unfortunately it fails to build with GCC which LLVM still tries to stay compatible with:
http://lab.llvm.org:8014/#/builders/16/builds/2211
gcc-11.0.0-0.19.fc34.x86_64
gcc-10.2.1-9.fc33.x86_64
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:86:8: error: explicit specialization of 'template<class T> struct llvm::DenseMapInfo' outside its namespace must use a nested-name-specifier [-fpermissive]

86 | struct DenseMapInfo<std::pair<const MCSymbol *, MCSymbolRefExpr::VariantKind>> {
   |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I have filed it to an existing GCC bugreport: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92598

I have committed 0e3d7e61867d69721b28e557272bdf4b66010327 in order to unblock the build on gcc builders.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

 #include <memory>
 #include <new>
 
 using namespace llvm;
 using namespace llvm::XCOFF;
 
 #define DEBUG_TYPE "asmprinter"
 
+// Specialize DenseMapInfo to allow
+// std::pair<const MCSymbol *, MCSymbolRefExpr::VariantKind> in DenseMap.
+// This specialization is needed here because that type is used as keys in the
+// map representing TOC entries.
+template <>
+struct DenseMapInfo<std::pair<const MCSymbol *, MCSymbolRefExpr::VariantKind>> {
+  using TOCKey = std::pair<const MCSymbol *, MCSymbolRefExpr::VariantKind>;
+
+  static inline TOCKey getEmptyKey() {
+    return {nullptr, MCSymbolRefExpr::VariantKind::VK_None};
+  }
+  static inline TOCKey getTombstoneKey() {
+    return {nullptr, MCSymbolRefExpr::VariantKind::VK_Invalid};
+  }
+  static unsigned getHashValue(const TOCKey &PairVal) {
+    return detail::combineHashValue(
+        DenseMapInfo<const MCSymbol *>::getHashValue(PairVal.first),
+        DenseMapInfo<int>::getHashValue(PairVal.second));
+  }
+  static bool isEqual(const TOCKey &A, const TOCKey &B) { return A == B; }
+};
+
 namespace {
 
 class PPCAsmPrinter : public AsmPrinter {
 protected:
-  MapVector<const MCSymbol *, MCSymbol *> TOC;
+  // For TLS on AIX, we need to be able to identify TOC entries of specific
+  // VariantKind so we can add the right relocations when we generate the
+  // entries. So each entry is represented by a pair of MCSymbol and
+  // VariantKind. For example, we need to be able to identify the following
+  // entry as a TLSGD entry so we can add the @m relocation:
+  //   .tc .i[TC],i[TL]@m
+  // By default, VK_None is used for the VariantKind.
+  MapVector<std::pair<const MCSymbol *, MCSymbolRefExpr::VariantKind>,
+            MCSymbol *>
+      TOC;
   const PPCSubtarget *Subtarget = nullptr;
   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(const MCSymbol *Sym);
+  MCSymbol *lookUpOrCreateTOCEntry(const MCSymbol *Sym,
+                                   MCSymbolRefExpr::VariantKind Kind =
+                                       MCSymbolRefExpr::VariantKind::VK_None);
 
   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(const MCSymbol *Sym) {
-  MCSymbol *&TOCEntry = TOC[Sym];
+MCSymbol *
+PPCAsmPrinter::lookUpOrCreateTOCEntry(const MCSymbol *Sym,
+                                      MCSymbolRefExpr::VariantKind Kind) {
+  MCSymbol *&TOCEntry = TOC[{Sym, Kind}];
   if (!TOCEntry)
     TOCEntry = createTempSymbol("C");
   return TOCEntry;
 }
 
 void PPCAsmPrinter::emitEndOfAsmFile(Module &M) {
   emitStackMaps(SM);
 }
auto getTOCRelocAdjustedExprForXCOFF = [this](const MCExpr *Expr,
     return MCBinaryExpr::createAdd(
         Expr, MCConstantExpr::create(-Adjustment, OutContext), OutContext);
   };
 
   auto getTOCEntryLoadingExprForXCOFF =
       [IsPPC64, getTOCRelocAdjustedExprForXCOFF,
        this](const MCSymbol *MOSymbol, const MCExpr *Expr) -> const MCExpr * {
     const unsigned EntryByteSize = IsPPC64 ? 8 : 4;
-    const auto TOCEntryIter = TOC.find(MOSymbol);
+    const auto TOCEntryIter =
+        TOC.find({MOSymbol, MCSymbolRefExpr::VariantKind::VK_None});
     assert(TOCEntryIter != TOC.end() &&
            "Could not find the TOC entry for this symbol.");
     const ptrdiff_t EntryDistanceFromTOCBase =
         (TOCEntryIter - TOC.begin()) * EntryByteSize;
     constexpr int16_t PositiveTOCRange = INT16_MAX;
 
     if (EntryDistanceFromTOCBase > PositiveTOCRange)
       return getTOCRelocAdjustedExprForXCOFF(Expr, EntryDistanceFromTOCBase);
if (!TOC.empty()) {
     const char *Name = isPPC64 ? ".toc" : ".got2";
     MCSectionELF *Section = OutContext.getELFSection(
         Name, ELF::SHT_PROGBITS, ELF::SHF_WRITE | ELF::SHF_ALLOC);
     OutStreamer->SwitchSection(Section);
     if (!isPPC64)
       OutStreamer->emitValueToAlignment(4);
 
     for (const auto &TOCMapPair : TOC) {
-      const MCSymbol *const TOCEntryTarget = TOCMapPair.first;
+      const MCSymbol *const TOCEntryTarget = TOCMapPair.first.first;
       MCSymbol *const TOCEntryLabel = TOCMapPair.second;
 
       OutStreamer->emitLabel(TOCEntryLabel);
       if (isPPC64 && TS != nullptr)
         TS->emitTCEntry(*TOCEntryTarget);
       else
         OutStreamer->emitSymbolValue(TOCEntryTarget, 4);
     }
void PPCAIXAsmPrinter::emitEndOfAsmFile(Module &M) {
   OutStreamer->SwitchSection(getObjFileLowering().getTOCBaseSection());
 
   PPCTargetStreamer *TS =
       static_cast<PPCTargetStreamer *>(OutStreamer->getTargetStreamer());
 
   for (auto &I : TOC) {
     // Setup the csect for the current TC entry.
     MCSectionXCOFF *TCEntry = cast<MCSectionXCOFF>(
-        getObjFileLowering().getSectionForTOCEntry(I.first, TM));
+        getObjFileLowering().getSectionForTOCEntry(I.first.first, TM));
     OutStreamer->SwitchSection(TCEntry);
 
     OutStreamer->emitLabel(I.second);
     if (TS != nullptr)
-      TS->emitTCEntry(*I.first);
+      TS->emitTCEntry(*I.first.first);
   }
 }
 
 bool PPCAIXAsmPrinter::doInitialization(Module &M) {
   const bool Result = PPCAsmPrinter::doInitialization(M);
 
   auto setCsectAlignment = [this](const GlobalObject *GO) {
     // Declarations have 0 alignment which is set by default.