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

[David Blaikie]

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.

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. 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.

I'm still not clear on what this duplication represents/why it happens -
could you explain it?

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?
> > >
> > > 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)
> > >
> > > http://reviews.llvm.org/D5451
> > >
> > >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140922/0c80686e/attachment.html>