[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:
> > 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`?
> I don't think we would ever end up with this type of code considering the `.set_conditional` directive has limited uses outside of the compiler, similarly to `.lto_discard`, for example. That being said, I don't think we should unconditionally emit the assignment in your example.
> 
> Specifically, if `sym_comm_A` is emitted, we'll end up with the following code:
> ```
> .set sym_comm_E, sym_comm_A
> .no_dead_strip sym_comm_E
> ```
> Or possibly:
> ```
> .no_dead_strip sym_comm_E
> ...
> .set sym_comm_E, sym_comm_A
> ```
> I don't see how we'd end up losing the attribute in either case. Did you have a specific situation in mind where this might happen?
> 
> If `sym_comm_A` is not emitted, we'll still end up setting the attribute (and registering the symbol), which doesn't produce anything useful, but works nevertheless:
> ```
> .no_dead_strip sym_comm_E
> ```
> Did you have a specific situation in mind where this might happen?

I'd like to make sure we don't regress `.no_dead_strip` support for macos; theoretically CFI+thinLTO is supported there.  Perhaps it's a non-issue, and it's ok to remove the plumbing of `MCSymbolAttr` through `emitConditionalAssignment`. But if we keep the plumbing, it would be good to add a test for `.no_dead_strip`.  That is all. But I don't feel strongly about the plumbing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113613