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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:2957-2958
+    Out.emitAssignment(Sym, Value);
+    if (NoDeadStrip)
+      Out.emitSymbolAttribute(Sym, MCSA_NoDeadStrip);
+  }
----------------
samitolvanen wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > should the check on `NoDeadStrip` occur regardless of `Cond`?
> > I meant:
> > ```
> > if (Cond) {
> >   if (Value->getKind() != MCExpr::SymbolRef)
> >     return Error(ExprLoc, "expected identifier");
> > 
> >   Out.emitConditionalAssignment(Sym, Value);
> > } else
> >   Out.emitAssignment(Sym, Value);
> > if (NoDeadStrip)
> >   Out.emitSymbolAttribute(Sym, MCSA_NoDeadStrip);
> > ```
> > Rather than plumb the `MCSymbolAttr` through `emitConditionalAssignment` only to then call `emitSymbolAttribute` anyways.
> I could be misreading something, but looking at `emitSymbolAttribute` implementations, `MCELFStreamer::emitSymbolAttribute` at least registers the symbol when emitting the attribute, which is exactly what we're trying to avoid unless the target symbol is also emitted. Is there a reason we would want to emit an attribute for a symbol we might not emit at all?
Perhaps not.  It seems that `MCSA_NoDeadStrip` is specific to MachO though perhaps WASM is using it, too.

Looking at a digestable test case, `llvm/test/MC/MachO/comm-1.s`:

```
        .comm           sym_comm_B, 2
        .comm           sym_comm_A, 4
        .comm           sym_comm_C, 8, 2
        .comm           sym_comm_D, 2, 3

        .no_dead_strip sym_comm_C
```
what happens if we add:
```
  .set_conditional sym_comm_E, sym_comm_A
  .no_dead_strip sym_comm_E
```
shouldn't llc turn the above asm back into:
```
  .set sym_comm_E, sym_comm_A
  .no_dead_strip sym_comm_E
```
otherwise we might be dropping `.no_dead_strip sym_comm_E`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113613