Patch for mem2reg pass

[David Blaikie]

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
>>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: debug_value_mem2reg_phi.diff
Type: application/octet-stream
Size: 6740 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130714/81e8057b/attachment.obj>