<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 7, 2014 at 3:46 PM, Adrian Prantl <span dir="ltr"><<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div><br>
> On Oct 7, 2014, at 10:30 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Mon, Oct 6, 2014 at 5:50 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br>
><br>
>> On Sep 26, 2014, at 11:42 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>><br>
>><br>
>> On Thu, Sep 25, 2014 at 12:03 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br>
>><br>
>>> On Sep 25, 2014, at 10:27 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>>><br>
>>><br>
>>><br>
>>> On Mon, Sep 22, 2014 at 4:46 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br>
>>><br>
>>> > On Sep 22, 2014, at 4:33 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>>> ><br>
>>> ><br>
>>> ><br>
>>> > On Mon, Sep 22, 2014 at 4:27 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br>
>>> ><br>
>>> > > On Sep 22, 2014, at 3:28 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>>> > ><br>
>>> > ><br>
>>> > ><br>
>>> > > On Mon, Sep 22, 2014 at 3:14 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br>
>>> > ><br>
>>> > > > On Sep 22, 2014, at 2:18 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>>> > > ><br>
>>> > > > as for testing - assuming these duplicates get deduplicated elsewhere (could you explain where?)<br>
>>> > > There are two kinds of duplicates that appear in this case:<br>
>>> > > 1) identical SDDbgNode* (same pointer value)<br>
>>> > > 2) an equivalent SDDbgNode (distinct pointers, but otherwise a copy)<br>
>>> > > The first kind is automatically deduplicated because the Invalid flag is set after processing it the first time.<br>
>>> > > The second one will result in two duplicate DBG_VALUE intrinsics which will get coalesced by the backend.<br>
>>> > ><br>
>>> > > OK - perhaps you could explain how these two equivalent but distinct SDDbgNodes come into existence?<br>
>>> > ><br>
>>> > ><br>
>>> > > > then I doubt there's much to test - this is then just a perf optimization, not a change in any semantics.<br>
>>> > > I was mostly posting this here seeking absolution for not committing this with a testcase :-)<br>
>>> > > > 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.<br>
>>> > > 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.<br>
>>> > ><br>
>>> > > You can't cause two of these equivalent but distinct objects to be created with less than several thousand lines?<br>
>>> > ><br>
>>> > > That is a bit surprising/strange.<br>
>>> ><br>
>>> > 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++<br>
>>> ><br>
>>> > Right - usually I reduce the input source as far as possible. creduce ( <a href="http://embed.cs.utah.edu/creduce/" target="_blank">http://embed.cs.utah.edu/creduce/</a> ) 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).<br>
>>> ><br>
>>> > 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.<br>
>>><br>
>>> No, what I did was this: First I ran bugpoint on an LLVM that had an assertion firing when a double insertion happened.<br>
>>><br>
>>> 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.<br>
>>><br>
>>> 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).<br>
>>><br>
>>> And after all that you still had a several thousand line reproduction? :/<br>
>>><br>
>>> Is there any way you could provide a source code sample and I can see if I can creduce down to something more manageable?<br>
>><br>
>> Source code is attached to the PR if you want to play with it: <a href="http://llvm.org/bugs/show_bug.cgi?id=20893" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=20893</a><br>
>><br>
>> 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?<br>
>><br>
>><br>
>>><br>
>>> ><br>
>>> > 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.<br>
>>> 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.<br>
>>> > 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.<br>
>>><br>
>>> Sure! That’s what I hoped the outcome of this review would be :-)<br>
>>><br>
>>> 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.<br>
>><br>
>> 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?<br>
>><br>
>> 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.<br>
>><br>
>>> ><br>
>>> > I'm still not clear on what this duplication represents/why it happens - could you explain it?<br>
>>><br>
>>> 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.<br>
>>><br>
>>> Hopefully this'll be easier for me to look at/understand with a minimized example.<br>
>><br>
>> 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.)<br>
>><br>
>> 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").<br>
>><br>
>><br>
>>><br>
>>><br>
>>> ><br>
>>> > 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.<br>
>>> ><br>
>>> > -- adrian<br>
>>> ><br>
>>> > ><br>
>>> > ><br>
>>> > > thanks,<br>
>>> > > adrian<br>
>>> > > ><br>
>>> > > > ================<br>
>>> > > > Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6180<br>
>>> > > > @@ +6179,3 @@<br>
>>> > > > +      if (*Val == *V) return;<br>
>>> > > > +    Vals.push_back(V);<br>
>>> > > > +  }<br>
>>> > > > ----------------<br>
>>> > > > Could 'Vals' be an actual set of SDDbgValue*s?<br>
>><br>
>> 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.<br>
>><br>
>>> > > ><br>
>>> > > > 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)<br>
>><br>
>> No, pointer identity is not sufficient. As mentioned earlier, we encounter both kinds of duplicates (pointer and deep copies) in the wild.<br>
>>><br>
>>> 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)<br>
>><br>
>> That sounds interesting, thanks!<br>
>>><br>
>>> > > ><br>
>>> > > > <a href="http://reviews.llvm.org/D5451" target="_blank">http://reviews.llvm.org/D5451</a><br>
>>> > > ><br>
>>> > > ><br>
>>> > ><br>
>>> > ><br>
>>> ><br>
>>> ><br>
>>><br>
>>><br>
>><br>
>><br>
><br>
> Hi David,<br>
><br>
> Were you any more successful in reducing the example than me?<br>
><br>
> Sorry, no - rather slow to run.<br>
><br>
> If not, I’d like to be pragmatic here and treat this as a performance/OOM problem<br>
><br>
> 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.<br>
><br>
> 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.<br>
><br>
> 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?<br>
<br>
</div></div>The investigation of bugs will always be a matter of priorities, and I honestly cannot guarantee that any one of us will have time to look at this soon.<br></blockquote><div><br></div><div>This is the kind of technical debt I've been spending quite a bit of time paying off (take for example the saga written in the commit message for <span style="font-family:monospace">219215</span> - all bugs hidden behind some otherwise innoccuous code to check a condition. Various other bugs found in inlining/abstract definitions/etc with similar conditionals that should've been assertions and no one spent the time to apply the appropriate rigor). I don't think it's unreasonable to expect that we actually understand why a fix is the right one before (or at least in short order, after) making it.<br><br>I'm not saying we're not going to make mistakes in doing so - sometimes we'll find out later that another approach would've been a better idea, certainly. But to make a fix while acknowledging that we don't know why the situation arises seems problematic and not something I really want to propagate without any expectation of closure.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
A pragmatic solution will evidently improve the situation for our users (cf. the comments from FreeBSD in the PR). </blockquote><div><br></div><div>The problem with this approach is it leads to grafting hacks upon hacks that make the code even harder to understand and harder to maintain in a way that people actually have a reasonable mental model of what's going on. This is not a good place to be.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">In fact, there is a good chance that this change indeed is the correct solution, because AFAICT DAGCombine may merge nodes that are carrying identical debug information.<br></blockquote><div><br></div><div>It might well be the right solution. I'd like for us to understand that properly. Perhaps rope in someone more familiar with DAGCombine who might be able to explain why this situation arises (perhaps even provide a test case from that information, rather than from experimentation/manual (or automated) reduction).<br><br>(it might be reasonably possible to put the assertion in for non-duplication, and run that compiler over a more modest codebase (a clang selfhost) to see if it crops up there - rather than trying to derive it from the OOM example which takes a particularly long time to run, if at all)<br><br>- David</div></div></div></div>