[PATCH] D116821: [DebugInfo][InstrRef] Move instr-ref controlling flag out of TargetOptions

Sat Jan 22 10:57:24 PST 2022

dblaikie added a comment.

In D116821#3263418 <https://reviews.llvm.org/D116821#3263418>, @xbolva00 wrote:

> In D116821#3263011 <https://reviews.llvm.org/D116821#3263011>, @alexfh wrote:
>
>> Hi Jeremy,
>> after this patch we see significantly increased compile-time memory usage, which makes a large number of targets to fail to compile with the (already quite large) memory limit we have on our build cluster.  I'll try to get specific numbers and maybe an example, but it seems like this patch has raised enough concerns to maybe flip the default back while all of these are being addressed?
>
> He explained that there is nothing much what is possible to do - you get better debug info but also CT and MU is a bit worse. But if I want debug info (-g), I want the best possible one, so …
>
> Why to pull every useful work out of trunk just because somebody has some issue? There is a flag, consider using it.

For what it's worth, this comes off a bit snarky/hyperbolic.

There's certainly tradeoffs to be made - "no regression ever" isn't a useful stance (there's some amount of compile time, memory usage, etc, that folks would likely be willing to pay for some benefit), but equally there is some level of regression that's "too much".

I don't think anyone's intending to suggest this work is dead/must be thrown out, but if the regression is more than expected (that's not clear to me - I think this patch should've included more numbers about expected regressions, and those suggesting that it might be suitable to revert should be documenting the level of regressions they're seeing - hopefully demonstrating more significant regressions than what's expected) it's probably reasonable to revert, analyze, and see if there are some extreme cases that weren't encountered/understood before the patch was committed, etc.

@alexfh - do you have some numbers to help clarify how significant these regressions are?
@jmorse - do you have some numbers to help clarify what sort of regressions you were anticipating this patch causing?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116821