Patch for mem2reg pass

[David Blaikie]

Oh, also - the change conceptually looks OK to me, but that's true of
many things. I'm not an expert at mem2reg and I'd like someone who is
to take a look at this before it's committed to make sure this makes
sense/is the simplest/consistent way to do this. We tend to end up
grafting on side-structures such as this map in many places because
people take a very narrow view of how to fix the issue without
considering how it might integrate into the surrounding code/algorithm
- I want to avoid that kind of approach as much as possible, which
means involving domain experts/owners of the code in such changes
rather than having debug info maintainers graft things into random
parts of the compiler we (or at least I) know little/nothing about.

On Sun, Jul 14, 2013 at 11:31 AM, David Blaikie <dblaikie at gmail.com> wrote:
> On Sat, Jul 13, 2013 at 5:12 AM, Piotr Perek <pperek1234 at gmail.com> wrote:
>> On 12.07.2013 23:50, David Blaikie wrote:
>>>>
>>>> The mem2reg pass loses the debug info for the affected variables. I have
>>>> created a patch to fix it. Please review.
>>>
>>> Thanks for working on this!
>>>
>>> The patch seems fairly simple, though I'm unfamiliar with that area of
>>> the code & can't sign off on it immediately.
>>>
>>> For testing - we're trying to move to a model in which we simplify C++
>>> (or C) source code as much as possible, compile that with Clang & use
>>> it as a test case (which it looks like you've done, more or less - it
>>> could, perhaps, be simpler source, though? (do you need a loop to
>>> demonstrate the problem?) - except you're missing the original C++
>>> source in your test case).
>>>
>>> -> Could you simplify the source and add it in comments to the test case?
>>
>> I added the source code to the test file as you asked.
>
> Thanks (nitpick, not necessary - format as the rest of LLVM code is
> (braces at the end of a line, rather than the beginning) & possibly
> place the source file somewhere generic like /tmp when compiling so we
> don't end up with all sorts of fun/interesting personal paths in the
> test cases (I don't always remember to do this, but I'm trying to make
> a habit of it))
>
>>
>>>
>>> Would it be beneficial to constrain the check any further than simply
>>> "there is a dbg.value for i.0"? Maybe where it exists in the code?
>>> What about other i.N SSA values produced by mem2reg? Or is that the
>>> only one?
>>
>> The dbg.value call is added after each new PHI Node created by the mem2reg
>> pass. It should be placed after the line defining the PHI Node. I think it
>> is checked in this test. What additional tests should I add?
>
> Just for completeness you could check that the debug metadata
> referenced by the dbg.value is the same metadata that defines the "i"
> variable. (use a FileCheck capture (like this: [[I_VAR:![0-9]*]] ) to
> capture the metadata number, then later on verify that that metadata
> number has the debug info for the variable (something like [[I_VAR]] =
> {{.*}} ; [ DW_TAG_auto_variable ] [i] or whatnot))
>
> Running this against my build I found a couple of problems, I've fixed
> one in a naive way (checking that the map contains the element rather
> than just accessing/default constructing it) - this failure came about
> by running "make check-all" on my build tree (where I have compiler-rt
> checked out, which is compiled by the fresh-built Clang, this tends to
> exercise a lot of code paths that no one bothered to write regression
> tests for). We (which is to say you, unless I find some more time to
> prioritize this) should modify the existing test case (or add another
> case to the same file, or another file if necessary) to exercise this
> failure, rather than relying on the compiler-rt build to catch it.
>
> Also, the compiler-rt build is still failing with this change after my
> first fix - seems to be some problems related to landing pads that I
> haven't figured out yet.
>
> If you don't want/need to check out compiler-rt, you can probably
> find/investigate/iron-out some of these issues by simply bootstrapping
> LLVM (build LLVM with the just-built Clang). If you can't reproduce
> the failures I've described with that (or resorting to checking out
> compiler-rt), let me know & I can try to provide reproductions for you
> to investigate & reduce.
>
>>
>>
>>>
>>>> Best regards,
>>>> Piotr
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>