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

Fri Sep 26 11:42:53 PDT 2014

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
>> > > >
>> > > >
>> > >
>> > >
>> >
>> >
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140926/e998fb2f/attachment.html>