<div dir="ltr"><div dir="ltr">On Fri, Sep 24, 2021 at 2:13 PM Arthur Eubanks <<a href="mailto:aeubanks@google.com">aeubanks@google.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Adding a map of mangled names to source locations (only those emitted in the IR since those are the only ones the backend can see) doesn't noticeably impact <a href="https://llvm-compile-time-tracker.com/compare.php?from=ac51ad24a75c02152f8ece943d65de9a1c4e947a&to=fd8cfa1e33bfce6421b3a088b859a240b056be5a&stat=instructions" target="_blank">compile time</a>, but does noticeably increase <a href="https://llvm-compile-time-tracker.com/compare.php?from=ac51ad24a75c02152f8ece943d65de9a1c4e947a&to=fd8cfa1e33bfce6421b3a088b859a240b056be5a&stat=max-rss" target="_blank">memory usage</a>. Of course, we'll win that back with clearing the Clang AST.</div></div></blockquote><div><br></div><div>I guess most of that is from the string data itself? Any chance that could be shared - by referencing (using StringRef) string data in the llvm::Module or otherwise? (bit more nuanced than that because presumably it'd be a problem if the string data were to go away - if a function is optimized out of an llvm::Module, etc - even if that meant the string value would never be queried for in the map anyway, it'd still violate some map invariants/etc)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>I found the recently implemented "dontcall" attribute which relies on the Clang AST. I have a draft patch to make it not rely on the Clang AST: <a href="https://reviews.llvm.org/D110364" target="_blank">https://reviews.llvm.org/D110364</a>.<br></div><div>But that and the changes in <a href="https://reviews.llvm.org/D109781" target="_blank">https://reviews.llvm.org/D109781</a> highlight an issue in that we don't only use the Clang AST to find source locations, we might also use it for other things.</div></div></blockquote><div><br></div><div>All the more reason to make the AST deleting mode the default (maybe the only mode, if possible)/usable everywhere - to avoid building new features that rely on the AST. (generally this makes situations like LTO better anyway - since they also break the idea that the AST is available during transformations)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>For example, we have custom naming of functions/lambdas/etc when printing out their name in a diagnostic with the Clang AST node. I've worked around this by calling LLVM's demangler, but it doesn't produce the same naming.<br></div><div>And for the "dontcall" attribute, the initial implementation looked into the Clang AST to determine whether to emit a warning or an error. I've worked around this by splitting it into two attributes, one for warning and one for error.</div><div>But overall, if we go through with this, we'll end up not having access to the Clang AST for future backend diagnostics. That's a tradeoff we'll have to decide on. Forcing this does make backend diagnostics more likely to be consistent when they don't have a Clang AST (e.g. ThinLTO post-link compiles, IR as input to Clang).</div></div></blockquote><div><br></div><div>yep!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 21, 2021 at 4:28 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</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"><div dir="ltr"><div dir="ltr">On Tue, Sep 21, 2021 at 4:25 PM Arthur Eubanks <<a href="mailto:aeubanks@google.com" target="_blank">aeubanks@google.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>But yeah - we do have the <a href="https://reviews.llvm.org/D4234" target="_blank">https://reviews.llvm.org/D4234</a> "LocTrackingOnly" mode which looks like it could be used for Rpass diagnostics, for instance. (& removing use of the AST from the LLVM diagnostic system might help make it more consistent behavior even when doing LTO or other separations between AST parsing and IR transformations)<br></div></div></div></blockquote><div><br></div><div>Thanks for the pointer to that. I was getting confused because "-Rpass=foo" was triggering LocTrackingOnly but "-Rpass" wasn't, which I've fixed. So -Rpass is no longer a concern.</div><div><br></div><div>So we just have to worry about non-Rpass diagnostics, e.g. -fwarn-stack-size. All of these only use the function name for source locations. We can either just give up and have worse diagnostics for these, meaning no demangling and no source location per diagnostic, or create a side table of these right before codegen as you suggested. For now I'll go with creating a side table to preserve the status quo, hopefully its construction runtime is not measurable.</div><div><br></div><div>Inline asm is special in that it carries around a source location token even in the IR, so we don't need to go through the AST to find a source location which is nice</div></div></div></blockquote><div><br>That all sounds pretty good to me - thanks for looking into all these nooks and crannies! </div></div></div>
</blockquote></div>
</blockquote></div></div>