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

[Baptiste Saleil via Phabricator via llvm-commits]

bsaleil marked 3 inline comments as done.
bsaleil added inline comments.


================
Comment at: llvm/include/llvm/MC/MCExpr.h:421
+// Specialize DenseMapInfo to allow MCSymbolRefExpr::VariantKind in DenseMap
+template <> struct DenseMapInfo<MCSymbolRefExpr::VariantKind> {
+  static inline MCSymbolRefExpr::VariantKind getEmptyKey() {
----------------
sfertile wrote:
> bsaleil wrote:
> > sfertile wrote:
> > > 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>`?
> > There is a generic implementation of `DenseMapInfo<std::pair<T, U>>` in `DenseMapInfo.h` which relies on `DenseMapInfo<T>` and `DenseMapInfo<U>`.
> 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.
Sure, I just updated the patch to make that change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96346/new/

https://reviews.llvm.org/D96346