[PATCH] [PR20893] Change SelectionDAG's DbgValueMap to have set semantics (NFC)

Tue Oct 7 10:30:51 PDT 2014

On Mon, Oct 6, 2014 at 5:50 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> On Sep 26, 2014, at 11:42 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Thu, Sep 25, 2014 at 12:03 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
>>
>> On Sep 25, 2014, at 10:27 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Mon, Sep 22, 2014 at 4:46 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>
>>>
>>> > On Sep 22, 2014, at 4:33 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>> >
>>> >
>>> >
>>> > On Mon, Sep 22, 2014 at 4:27 PM, Adrian Prantl <aprantl at apple.com>
>>> wrote:
>>> >
>>> > > On Sep 22, 2014, at 3:28 PM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>> > >
>>> > >
>>> > >
>>> > > On Mon, Sep 22, 2014 at 3:14 PM, Adrian Prantl <aprantl at apple.com>
>>> wrote:
>>> > >
>>> > > > On Sep 22, 2014, at 2:18 PM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>> > > >
>>> > > > as for testing - assuming these duplicates get deduplicated
>>> elsewhere (could you explain where?)
>>> > > There are two kinds of duplicates that appear in this case:
>>> > > 1) identical SDDbgNode* (same pointer value)
>>> > > 2) an equivalent SDDbgNode (distinct pointers, but otherwise a copy)
>>> > > The first kind is automatically deduplicated because the Invalid
>>> flag is set after processing it the first time.
>>> > > The second one will result in two duplicate DBG_VALUE intrinsics
>>> which will get coalesced by the backend.
>>> > >
>>> > > OK - perhaps you could explain how these two equivalent but distinct
>>> SDDbgNodes come into existence?
>>> > >
>>> > >
>>> > > > then I doubt there's much to test - this is then just a perf
>>> optimization, not a change in any semantics.
>>> > > I was mostly posting this here seeking absolution for not committing
>>> this with a testcase :-)
>>> > > > Maybe dumping MI could be testable, but I'm not sure what the
>>> dumping options (& whether they're available in non-asserts builds, etc)
>>> are like.
>>> > > There is -print-after-all and the likes, but the real problem is
>>> that the testcase is too large to serve as a useful regression test.
>>> > >
>>> > > You can't cause two of these equivalent but distinct objects to be
>>> created with less than several thousand lines?
>>> > >
>>> > > That is a bit surprising/strange.
>>> >
>>> > You’re probably aware that constructing test cases from scratch is
>>> pretty hard to do manually when debug info is involved. Employing bugpoint,
>>> delta and a python script that eliminates unused MDNodes, I was able to
>>> reduce 100k+ lines of C++
>>> >
>>> > Right - usually I reduce the input source as far as possible. creduce
>>> ( http://embed.cs.utah.edu/creduce/ ) usually gets me down to a couple
>>> of hundred lines. Though I realize something like this might not reduce so
>>> easily (one way to workaround this is sometimes to add more robust
>>> assertions (eg: asserting that you don't add the same element twice would
>>> give you a reproduction of a case where that second element is added),
>>> causing the failure to be more reliable/happen earlier - thus the reduction
>>> is faster and narrower).
>>> >
>>> > Were you reducing the OOM directly? (ie: the test you gave delta
>>> involved "check that the program still OOMs"?) Yeah, that's no fun. I've
>>> reduced an unreliable repro a few weeks back which took a couple of days to
>>> get down to something more manageable.
>>>
>>> No, what I did was this: First I ran bugpoint on an LLVM that had an
>>> assertion firing when a double insertion happened.
>>
>>
>> Ah - I generally use delta (or more accurately/effectively, CReduce) on
>> the original source code rather than bugpoint - given the gnarly issues
>> involved with mutating IR leading to debug info metadata invariant
>> violations, etc.
>>
>>
>>> After bugpoint was done I ran delta on the resulting IR, with two
>>> versions LLVM, one with the assertion, and one with my patch applied. I
>>> would have delta only accept revisions that were the assertion fired, and
>>> the patch made the program compile fine (this is essential when reducing
>>> metadata, otherwise it’s too easy to come up with an input that will
>>> trigger the error, but won’t compile once the bug is fixed).
>>>
>>
>> And after all that you still had a several thousand line reproduction? :/
>>
>> Is there any way you could provide a source code sample and I can see if
>> I can creduce down to something more manageable?
>>
>>
>> Source code is attached to the PR if you want to play with it:
>> http://llvm.org/bugs/show_bug.cgi?id=20893
>>
>
> Guess I need a release+asserts for this. The debug+asserts build I started
> building yesterday is still building (just one build, not even
> multiple/delta/creduce). Could we add the assertion and find a
> smaller/shorter running test case? How long did it take you to
> compile/assert/reproduce this failure?
>
>
>>
>>
>>
>>> >
>>> > If it's just about the OOM, and the OOM is because of these duplicates
>>> - I'd consider putting in an assertion that would fire when the duplicates
>>> appear, and just reducing that instead - it should be much faster, more
>>> reliable, and smaller.
>>> That’s precisely what I did (in my first step). In the second step I ran
>>> delta on the IR (as outlined above). In the third step I ran a python
>>> script that would compute the transitive closure of all MDNodes referenced
>>> by the IR file (sans MDnodes) and strip all metadata nodes that were no
>>> longer needed.
>>> > There's no need to create or commit a test case that used to OOM - we
>>> don't need anything that big in the regression suite and the same codepath
>>> can be exercised by a much smaller example that just shows even a single
>>> case of the duplication that's of interest.
>>>
>>> Sure! That’s what I hoped the outcome of this review would be :-)
>>>
>>
>> While I'm totally with you that committing an example that used to
>> actually OOM the compiler isn't helpful, adding an assertion to help reduce
>> down a test case that exercises the set-like behavior seems relevant and
>> should be far smaller/more reasonable to commit. I'd like to have something
>> in-tree that has the potential to highlight the difference in behavior
>> (even if it's just a difference in performance) between a set and a vector.
>>
>>
>> I’d like to suggest that I commit one change that introduces the
>> assertion, followed by one that changes it to a set. Then, someone with a
>> better understanding of the problem (you, my future self) finds a
>> fundamental underlying issue we can revisit the set/vector decision?
>>
>
> I really don't like to paper over problems without understanding them - it
> makes the codebase harder to understand and thus harder to make future
> changes. I'd really like to make sure we frontload understanding the
> underlying problem (even if the conclusion is exactly the same solution as
> you're proposing) rather than leaving it to some non-specific point in the
> future.
>
>
>> >
>>> > I'm still not clear on what this duplication represents/why it happens
>>> - could you explain it?
>>>
>>> As far as I understand it so far, SelectionDAG is combining two SDNodes
>>> and then merging their DbgValues via DAG.TransferDbgValues(). Both SDNodes
>>> to be combined have an SDDbgValue with identical contents, both get
>>> appended to the resulting SDNode’s DbgValueMap entry.
>>>
>>
>> Hopefully this'll be easier for me to look at/understand with a minimized
>> example.
>>
>>
>> Believe me that’s what I was hoping for, too, and I was quite
>> disappointed when the code was still so long. Maybe creducing the source is
>> more effective. (I usually don’t even bother reducing the source for large
>> preprocessed C++ projects because a single pass through the frontend is
>> already so expensive.)
>>
>
> creduce can certainly take hours/days (well, it took days when I had a
> flakey failure and thus my "test" was "run the compiler 20 times and see if
> it fails in any of those 20 times").
>
>
>>
>>
>>
>>>
>>> >
>>> > to 2375 lines of IR (2139 of which are metadata!), but I failed to go
>>> beyond that (see also the referenced PR). The function in the reduced
>>> testcase contains a single dbg.value intrinsic and a really long chain of
>>> GEP instructions.
>>> >
>>> > -- adrian
>>> >
>>> > >
>>> > >
>>> > > thanks,
>>> > > adrian
>>> > > >
>>> > > > ================
>>> > > > Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6180
>>> > > > @@ +6179,3 @@
>>> > > > +      if (*Val == *V) return;
>>> > > > +    Vals.push_back(V);
>>> > > > +  }
>>> > > > ----------------
>>> > > > Could 'Vals' be an actual set of SDDbgValue*s?
>>>
>>
>> I was concerned that that would use more memory, but really most of the
>> times there should be only one SDDbgValue in there anyway. The set would
>> certainly be cleaner. I’m happy to change this. Less code = fewer bugs.
>>
>> > > >
>>> > > > Are SDDbgValue*s inherently uniqued such that pointer identity
>>> would be sufficient? (or do you need that equality comparison added above?
>>> Which could hopefully be adjusted to < comparison (or hashing) for a
>>> set-like structure)
>>>
>>
>> No, pointer identity is not sufficient. As mentioned earlier, we
>> encounter both kinds of duplicates (pointer and deep copies) in the wild.
>>
>>
>> Question here still /\, (as an aside, if you need a convenient
>> implementation of < you can use std::tie(...) < std::tie(...) to use
>> tuple's tie to implement tie of some arbitrary things - though it'll still
>> require some tricks due to the union/variant aspect of this structure)
>>
>>
>> That sounds interesting, thanks!
>>
>>
>>
>>> > > >
>>> > > > http://reviews.llvm.org/D5451
>>> > > >
>>> > > >
>>> > >
>>> > >
>>> >
>>> >
>>>
>>>
>>
>>
>
> Hi David,
>
> Were you any more successful in reducing the example than me?
>

Sorry, no - rather slow to run.

> If not, I’d like to be pragmatic here and treat this as a performance/OOM
> problem
>

It certainly is a performance/OOM problem, but the solution sounds like a
hack if we don't actually know why this situation is arising. I don't want
such a thing to live in the codebase very long.

> and change the implementation to set semantics now rather than leaving it
> unfixed. We can still leave the PR open for determining how deep copies
> come into existence to see if there is an unresolved underlying problem.
>

In many cases these bugs seem to go uninvestigated. I don't want that to be
the case here. Is there any reason to believe it won't be?

- David

>
> -- adrian
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141007/f6c93d94/attachment.html>