[PATCH] D143229: [AutoUpgrade] Add flag to disable auto-upgrade

Tue Feb 28 11:02:28 PST 2023

mehdi_amini added inline comments.

================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:88
+    cl::desc("Assume that imported functions are built at the same revision of "
+             "LLVM as this consumer to avoid upgrading debug info"));
+
----------------
aeubanks wrote:
> mehdi_amini wrote:
> > aeubanks wrote:
> > > mehdi_amini wrote:
> > > > I'll repeat my previous earlier comment: that seems to me like a very ad-hoc flag for some overly specific use-case.
> > > I wouldn't consider this use case "overly specific", I'd guess that many uses of LLVM don't care about bitcode upgrading.
> > > 
> > > Given that it seems quite hard to put a consistent LLVM revision in the bitcode that can be understood everywhere, I think this is something that requires a flag and bitcode version guarantees at a higher level.
> > > 
> > > There are two options I can think of:
> > > 1) under a flag, when producing bitcode, add a marker saying "this bitcode will never need to be upgraded", then when seeing that marker when importing, don't upgrade debug info
> > > 2) under a flag, when reading/importing bitcode, assume it doesn't need to be upgraded (this patch)
> > > 
> > > 1 seems more annoying than 2
> > > 
> > > do you have any other alternatives?
> > You're presenting alternatives as if this is the only possibility. Doing nothing is another possibility for example...
> > 
> > Adding flag is not free: I see it as bad practice in general, and an easy "no design" solution that is too often overlooking the general cost of supporting matrix of configurations. There should be a **high** bar for this kind of things.
> > 
> > It's not clear to me that there is a strong case here to justify a flag in the first place, and especially I haven't seen any other area explored: to begin with "why is it costly and what can we do to optimize it".
> The "why is this costly" part was answered by aprantl (need to run the verifier to detect cycles)
> 
> I'm not understanding the matrix of configurations support argument, this flag doesn't really interact with anything else.
> 
> For background, we spend about 2% of our overall compile time on autoupgrade because we do a lot of ThinLTO builds. 2% spent on code that doesn't need to run is pretty high and I think that deserves some sort of change in LLVM.
> 
> I agree that adding a flag is not ideal, but ultimately LLVM has a bitcode compatibility guarantee that isn't going to change. Assuming some subset of people want to avoid some of these autoupgrade costs if they have bitcode version guarantees, we have to have a flag somewhere since the default assumption is that old bitcode is upgradable. We could explore things like having an unstable bitcode format that only works at a single LLVM revision. But this is a nice stopgap in the meantime.
> The "why is this costly" part was answered by aprantl (need to run the verifier to detect cycles)

I'm not totally seeing any satisfactory answer here: "it is costly because it does 'work'" is a bit under-detailed in terms of analysis. For example: what would it take to make it faster?

> 2% spent on code that doesn't need to run is pretty high and I think that deserves some sort of change in LLVM.

Again, you're eliding here that you're in a very niche use-case: ThinLTO in a very controlled build environment. Are there other identified users than Google on which we know this will have a significant impact that justified adding a flag in LLVM?

It still overall comes across to me like "reaching for the easiest stopgap" without due diligence on overall design exploration and mitigation (like optimizing the upgrade, embedding the information differently,  etc.). 
@aprantl  mentioned in the first comment in this revision that if you need a stopgap for your own need, you can easily add this downstream.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143229