[PATCH] Bugfix for debug intrinsic handling in InstCombiner

[Bob Wilson]

On Apr 16, 2013, at 10:56 AM, Eric Christopher <echristo at gmail.com> wrote:

> I'm not sure it's straightforward at all please tell me why you think so?

I meant that the code in the patch is straightforward.  It is simply checking if a debug intrinsic call already exists and avoiding a duplicate if it's already there.  It sounds like you question whether this is the most general solution, but it's fixing a serious bug and is not likely to break anything, nor is it a new feature that is going to have implications throughout the compiler..

I'm sure Adrian will respond to answer your questions about the circumstances that led to this happening.  If you have an idea for a more general solution, that would be great.  Until we have a more general solution, this is a safe fix that makes the situation better than it was before.

> 
> This comes down to whether or not the fix is best done in
> LowerDbgDeclare or elsewhere. I'm guessing what's happening is that
> the alloca for the original loads and stores have been nuked (likely
> by mem2reg) and therefore the assumptions through large portions of
> the backend aren't holding anymore. These assumptions are an
> unfortunate problem we've inherited. I don't have any visibility about
> how the testcase was produced (and yes, it definitely needs cleanup)
> and don't know how the IR got into the current state. I completely
> agree there's a lot of nasty problems in how we deal with non-alloca
> variables and value tracking. What's your longer term plan for dealing
> with the problem more generally?
> 
> Thanks!
> 
> -eric
> 
> On Tue, Apr 16, 2013 at 9:50 AM, Adrian Prantl <aprantl at apple.com> wrote:
>> 
>> On Apr 16, 2013, at 9:25 AM, Bob Wilson <bob.wilson at apple.com> wrote:
>>> 
>>> The second patch is also OK.  If you can do anything to pare down the test case, especially the metadata, please do.  This could have been reviewed after it was committed, since you're just fixing a bug and the fix is straightforward.
>> 
>> That’s fine. I can commit straightforward bug fixes directly and have them reviewed after commit. I assume Eric (as the code owner) will let me know if my definition of “straightforward” differs from his :-)
>> 
>> -- adrian