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

[Sami Tolvanen via Phabricator via llvm-commits]

samitolvanen added inline comments.


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:2957-2958
+    Out.emitAssignment(Sym, Value);
+    if (NoDeadStrip)
+      Out.emitSymbolAttribute(Sym, MCSA_NoDeadStrip);
+  }
----------------
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
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113613