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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

In chrome, we add a clang `-D` flag containing the toolchain revision for caching/determinism reasons so we can also guarantee that bitcode inputs will always match.

I may be wrong, but I believe rust does inter-crate (Thin)LTO, all bitcode produced from the same rustc frontend, which could also use this.

In D143229#4126191 <https://reviews.llvm.org/D143229#4126191>, @tejohnson wrote:

> @aeubanks are we also invoking UpgradeDebugInfo, or the Verifier separately, on the main input bitcode in the distributed ThinLTO backend clang invocation (e.g. the clang -x ir input file)? For the same reason, we should be able to skip that. So I wonder if it makes sense then for the disabling mechanism (e.g. flag or whatever) to be in UpgradeDebugInfo or the Verifier itself?

Yeah there is some time spent verifying the main input bitcode (although much less than function importing). Moved the flag to autoupgrade.



================
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"));
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143229