<div dir="ltr"><div>I like the second option. It addresses the original issue and removes any fragility. I think option 3 invalidates the contract analyses have with the system (i.e. IR mutation => analysis invalidated), and we shouldn't take it, it leads to fragility and maintenance costs, and I offer the relatively recent memory of the cgscc pass and immutable analyses and invalidation as illustration of how not fun diagnosing these kinds of issues are.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 10, 2021 at 11:25 AM Arthur Eubanks via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">aeubanks added a comment.<br>
<br>
(moving this to a main thread so we don't have to reply to a thread on code in an old revision)<br>
<br>
hmm, does removing dead constant users invalidate analyses? that's an interesting question. I guess technically yes it should, although not sure it'll matter in practice.<br>
<br>
I think we have three options<br>
<br>
1. mtrofin's original idea of adding a cleanup pass before the inliner that calls removeDeadConstantUsers on each function, and we invalidate non-cfg analyses if we make any changes<br>
2. add `Constant::hasOneLiveUse()` and use that in InlineCost.cpp rather than modifying IR<br>
3. keep doing what we're doing here and assume that no analyses rely on dead constant users<br>
<br>
imo we can get away with the current patch<br>
any thoughts?<br>
<br>
<br>
Repository:<br>
  rG LLVM Github Monorepo<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D109294/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D109294/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D109294" rel="noreferrer" target="_blank">https://reviews.llvm.org/D109294</a><br>
<br>
</blockquote></div>