[Mlir-commits] [lldb] [llvm] [mlir] [DebugInfo] Add symbolic branches to DIExpression (PR #210850)

[Scott Linder]

slinder1 wrote:

> > After re-reading the other review comments, and then actually taking a look at the code, I have some concerns I want to hash out. I don't know how to bring some of this up without coming off as accusatory, so I will just say from the start that I don't have any ill-will, I am just worried about some patterns I'm seeing and want to discuss them openly.
> 
> I appreciate the candid feedback.
> 
> > First, it seems like the patch itself and all of the initial wave of accepting code reviews are coming from devs working at NVIDIA, maybe even on the same internal team?
> 
> Not entirely, but some of us work together
> 

I wish I had a better way to couch the concerns, as I don't mean to imply any sort of malice on anyone's part here. I just think there is the potential for devs who work closely to give each other the "benefit of the doubt" in reviewing.

That possibility, coupled with valid review concerns going unmentioned by several reviewers just feels like something that needs to be addressed.

> > If that's true, it worries me that concerns from @felipepiovezan, @Snowy1803 and @jmorse did not seem to get addressed before those reviews came in, and the reviews don't make any mention of them.
> 
> I did try to reply to each of them as they came in. Some I disagreed with and some I likely have agreed with as I haven't landed this yet.

That the change hasn't landed doesn't affect the bits I'm most concerned about. Ultimately the review is in a state now where there are 4 unqualified "LGTM"s interspersed with valid review concerns. How we ended up in that state, when I don't think any individual participant wanted us to, is the problem I want to address.

Put another way, my issues are with the net effect of the reviews from @bzcheeseman, @ayermolo, @enferex, and @dwblaikie which (however inadvertently) feel like they all either:

* Were submitted without seeing the concerns from several other devs, or
* Were submitted after seeing those concerns, but without qualifying the acceptance on them being addressed.

In either case, I worry that it is then implicitly incumbent upon e.g. @felipepiovezan to keep pushing on their (very sensible, IMO) concerns in the face of a tide of "LGTM" reviews.

I'll also note that the change being assisted by LLMs gives me some pause. I have become acutely aware of the potential for LLMs to "grease the wheels" in making fundamentally unsound changes easier to "stomach", both for the original developer and for those who review it.

For example, you responded to a concern around how this feature is being added in a state where code which currently always preserves the validity of expressions it touches _may suddenly start breaking valid expressions_ by suggesting we can just make it "incumbent on the producers", but I am fundamentally opposed to that approach. We should be working towards a sound, complete model for how the compiler can legally transform `DIExpression`s, and saying "if the input uses feature X, all bets are really off, and it is the producer's fault if there is a distant verifier failure" just seems unacceptable to me on its face.

We are unfortunately in a state where `DIExpression` today doesn't really have a great abstract model. This change (at least as-is) just seems to compound that issue in a new dimension, which I don't think is sustainable.

> The concerns about DIArglist were particularly noteworthy. I thought it was a good callout at the beginning to try to get some use and I'll take "let's not".

I'm having trouble parsing what you mean here, could you rephrase to try to help me?

> I do want to address their concerns and have added a couple of things to start doing that to the patch to make it clear.

That's great, and I do appreciate that you haven't landed the patch in its current state! I again don't mean to point fingers, nor do I just want to hold up improvements to the debug-info. I genuinely just see process failures and want to understand and address them.

> > It also seems like, from a high level, we should be considering what shape control-flow for debug-info should even take. As @adrian-prantl has mentioned before, the fact that we currently use not-quite-DWARF expressions as a stand-in for a "debug-info IR" is just a matter of convenience, weighed against the minimal benefits a more complete solution seemed to offer.
> > I think that calculus starts to change once we need control-flow. We already have work like `DIExpression::foldConstantMath`, which shows benefits to optimizing the intermediate debug-info, and the fact that this design seems to assume we will not want any control-flow sensitive optimization is not ideal.
> 
> It's an interesting perspective. I saw this more as a directionally correct (with caveats as called out) patch to start going down the path - we don't know what we want to optimize until we get a chance to try it out :)

True, we may not know the particulars, but we _are_ optimizing-compiler developers. It just seems like the path we are on, with a distinct pseudo-IR based on a virtual-machine assembly format (DWARF expressions) but used throughout all of compilation will not be a great vessel for doing control-flow sensitive optimizations.

The GCC representation for debug-info has been based on the same IRs (RTL and GIMPLE) they use for actual code for some time now. I don't know that we need to follow that (i.e. using LLVM IR and MIR) but I think we should explore something which gives us some of the same benefits, namely being able to:

* Implement `salvageDebugInfo` as basically "code motion", reusing e.g. removed `llvm::Instruction`s rather than translating to DWARF opcodes
* Represent control-flow in a first-class way that is already well understood
* Point existing optimization passes at debug-info expressions
* Use existing inlining/outlining techniques to automatically leverage DWARF subroutines/calls to optimize for e.g. debug-info size

For the Tile DSL case that seems to drive the need here, I think the more complex the initial expression has to be, the more useful all of this becomes. My (admittedly limited) understanding of how Tile DSLs are lowered is that certain things which are parametric in the DSL input become fixed as lowering approaches hardware, in which case optimizing the accompanying debug info to match seems potentially useful. The alternative seems like it would be to reconstruct the parametric input, only to then feed it through the unoptimized debug expression?

> That said, you have other feedback inline and I will address that. If you also think an RFC is the right place to start - I'd like to avoid redesigning DIExpression too much because I'm trying very hard to keep this as local to the expression infra as possible - at least within AutoUpgrade possibilities :)

I can definitely see us hashing out a higher-level path in an RFC that involves more fundamental work, while still deciding that a local change with caveats like this one is reasonable to go forward with in the mean-time. I do think we should aim to limit the "conceptual fallout" from that local change, in any case.

I am again not trying to just hold up progress for the sake of it, and I don't imagine anyone else who had/has reservations about this patch have that goal either.

https://github.com/llvm/llvm-project/pull/210850