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

[Adrian Prantl]

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