[PATCH] D113613: [ThinLTO][MC] Use conditional assignments for promotion aliases

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/include/llvm/MC/MCStreamer.h:241
+  /// Tracks the symbols we have emitted for conditional assignments.
+  StringMap<bool> emittedSymbols;
+
----------------
In general, a mapping of any key type to a boolean is a degenerate case of using a set.

ie. here, we have a mapping from string -> bool.  The same could be accomplished by simply using a set of strings; lack of appearance in the set corresponds to false in the mapping.

Either the key exists in the set, or it doesn't.  Unless you mean to track three states (true, false, not in mapping/set)?

https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringset-h


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:2957-2958
+    Out.emitAssignment(Sym, Value);
+    if (NoDeadStrip)
+      Out.emitSymbolAttribute(Sym, MCSA_NoDeadStrip);
+  }
----------------
should the check on `NoDeadStrip` occur regardless of `Cond`?


================
Comment at: llvm/lib/MC/MCStreamer.cpp:450-457
+  const MCSymbol &Target = cast<MCSymbolRefExpr>(*Value).getSymbol();
+
+  // If the symbol already exists, emit the assignment. Otherwise, emit it
+  // later only if the symbol is also emitted.
+  if (emittedSymbols.count(Target.getName()))
+    emitAssignment(Symbol, Value);
+  else
----------------
Looks like:
```
StringRef Target = cast<MCSymbolRefExpr>(*Value).getSymbol().getName();
```
might DRY up this code further?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113613