[PATCH] D143229: [FunctionImporter] Add flag to disable upgrading debug info

[Mehdi AMINI via Phabricator via llvm-commits]

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:
> > 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".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143229