<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Nov 12, 2014, at 11:02 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class="Apple-interchange-newline"><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Tue, Nov 11, 2014 at 8:57 PM, Frédéric Riss<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:friss@apple.com" target="_blank" class="">friss@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><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 style="word-wrap: break-word;" class=""><br class=""><div class=""><div class=""><div class="h5"><blockquote type="cite" class=""><div class="">On Nov 11, 2014, at 2:26 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""><div class=""><br class=""><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class=""><div class="gmail_quote" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;">On Tue, Nov 11, 2014 at 2:16 PM, Frédéric Riss<span class=""> </span><span dir="ltr" class=""><<a href="mailto:friss@apple.com" target="_blank" class="">friss@apple.com</a>></span><span class=""> </span>wrote:<br class=""><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 style="word-wrap: break-word;" class=""><br class=""><div class=""><blockquote type="cite" class=""><span class=""><div class="">On Nov 11, 2014, at 1:43 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""></span><div class=""><div class=""><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Tue, Nov 11, 2014 at 1:21 PM, Frederic Riss<span class=""> </span><span dir="ltr" class=""><<a href="mailto:friss@apple.com" target="_blank" class="">friss@apple.com</a>></span><span class=""> </span>wrote:<br class=""><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;">Author: friss<br class="">Date: Tue Nov 11 15:21:08 2014<br class="">New Revision: 221709<br class=""><br class="">URL:<span class=""> </span><a href="http://llvm.org/viewvc/llvm-project?rev=221709&view=rev" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=221709&view=rev</a><br class="">Log:<br class="">Totally forget deallocated SDNodes in SDDbgInfo.<br class=""><br class="">What would happen before that commit is that the SDDbgValues associated with<br class="">a deallocated SDNode would be marked Invalidated, but SDDbgInfo would keep<br class="">a map entry keyed by the SDNode pointer pointing to this list of invalidated<br class="">SDDbgNodes. As the memory gets reused, the list might get wrongly associated<br class="">with another new SDNode. As the SDDbgValues are cloned when they are transfered,<br class="">this can lead to an exponential number of SDDbgValues being produced during<br class="">DAGCombine like in<span class=""> </span><a href="http://llvm.org/bugs/show_bug.cgi?id=20893" target="_blank" class="">http://llvm.org/bugs/show_bug.cgi?id=20893</a><br class=""><br class="">Note that the previous behavior wasn't really buggy as the invalidation made<br class="">sure that the SDDbgValues won't be used. This commit can be considered a<br class="">memory optimization and as such is really hard to validate in a unit-test.<br class=""></blockquote><div class=""><br class="">This was sort-of undefined behavior previously (comparing a previously free'd pointer to a new allocation) but is still sort-of detectable. If we added an assertion into some point of initial construction or SDNode usage, that there were zero valuse in its map - then this assertion would fire on the buggy code? (but only when the allocation got lucky and put it in the same place) But would at least fail rather than behave strangely (leakily, mind you, but strangely)<br class=""><br class="">I imagine at the call to setHasDebugValue, if it previously had !getHasDebugValue, then the number of entries in the map should be 1 (this should be the first thing we've added to the map), something like changing this:<br class=""><br class=""><pre style="margin-top: 0px; margin-bottom: 0px; padding-top: 0.5em; font-size: inherit; line-height: 16.25px;" class=""><span style="display: block;" class=""> DbgInfo->add(DB, SD, isParameter);
</span><span style="display: block;" class=""> <span style="color: rgb(0, 0, 136);" class="">if</span> (SD)
</span><span style="display: block;" class=""> SD->setHasDebugValue(<span style="color: rgb(0, 0, 136);" class="">true</span>);</span></pre><br class="">to this:<br class=""><pre style="margin-top: 0px; margin-bottom: 0px; padding-top: 0.5em;" class=""><span style="font-size: inherit; line-height: 16.25px; display: block;" class="">
if (SD) {</span><span style="display: block;" class=""><font size="3" class=""><span style="line-height: 16.25px;" class=""> assert(DbgInfo-></span></font><font size="3" class=""><span style="line-height: 16.25px;" class="">getSDDbgValues(SD).empty() || !SD->getHasDebugValue());
SD->setHasDebugValue(true);
}
DbgInfo->add(DB, SD, isParameter);
</span></font><span style="color: rgb(34, 34, 34); font-size: small; line-height: normal; font-family: arial; white-space: normal;" class="">I agree that adding a test case may still be questionable, given the need for this to end up getting a matching allocation, but maybe there are some simple cases that pretty obviously get the same memory address because they're back-to-back release-then-reallocate? (I think I added a test case like this when I fixed some similar bugs in ArgPromo or DAE, maybe... one of them has a two-pass algorithm that had some invalidity going on - </span><font face="arial" class=""><span style="white-space: normal;" class=""><a href="http://llvm.org/viewvc/llvm-project?rev=219210&view=rev" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=219210&view=rev</a><span class=""> </span>)</span></font></span></pre></div></div></div></div></div></div></div></blockquote><div class=""><br class=""></div><div class="">Sorry, I missed that part of the mail. With the assert in place, it might be possible to come up with an IR that reproduces this behavior, but as it relies on allocation order it would be quite fragile I suppose (Note that I do know absolutely nothing about SDAG allocation strategy). I’ll try to toy with it when I find some time, but no promises…</div></div></div></blockquote><div class=""><br class="">Yep - see my example from ArgPromo - in that case an easy example existed due to the deallocation and immediate reallocation (of course this test case won't always exercise the problematic case - different memory allocators could easily defeat it - but it seems at least simple/plausible enough to include (adding appropriate comments & caveats to the test case in case it entirely rots, so someone knows they can remove it in that case))<br class=""><br class="">I'd expect the case where the allocation is reused is a fairly simple one like the ArgPromo issue I hit, but if it doesn't fall out so easily, certainly don't spend a lot of time on it. Though I wouldn't mind if you looked at it sooner rather than later - at least enough to decide one way or the other. Unspecified times in the future are easily forgotten (but you know, doesn't have to be today - but within a week or so seems reasonable, beyond that and I imagine it'll get lost).<br class=""></div></div></div></blockquote><div class=""><br class=""></div></div></div><div class="">So I came up with a small example that triggers the assert on the bus home. I’m still wondering if it’s worth committing. I think the test is fragile not because of the allocation strategy (SelectionDAG uses a Recycling allocator which is fairly predictable), but because it depends really a lot on the ISel/Combiner behavior in itself, and I suppose this is much more prone to change. Moreover it would be a test with just a RUN line, testing that it doesn’t crash which are usually frowned upon.</div></div></div></blockquote><div class=""><br class=""></div><div class="">It's usually frowned upon (usually by me - I don't see anyone else much caring) because it represents a lack of test coverage (program was crashing on some codepath, now it's not crashing - but presumably there was/is specific behavior other than "anything other than crash" that we were expecting) - as you say, other codepaths ensured that this bad behavior didn't leak out anywhere, but we could still test the output on this codepath to make sure it's the obviously right thing.</div><div class=""> </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;"><div style="word-wrap: break-word;" class=""><div class=""><div class="">That said I’m happy to commit it along with the assert if you think it’s worth it.<br class=""></div></div></div></blockquote><div class=""><br class="">Either way, the assertion's probably worth adding I imagine. </div></div></div></blockquote><div><br class=""></div><div>No questions, the assertion will go in anyway.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class="">I wouldn't mind at least seeing the test case - either in code review/just an attached patch, or committed (though do include your comment about what makes this test brittle/likely to become invalid later - and if you do commit it, the commit message should reference the revision with the fix so that should someone wonder if the test is still meaningful, they can locally revert the fix and see if the test still fails)<br class=""></div></div></div></blockquote><div><br class=""></div><div>I haven’t turned it into a FileCheck test, but this is the base code:</div><div><div style="margin: 0px;" class=""><br class=""></div><div style="margin: 0px;" class="">#include <stdint.h></div><div style="margin: 0px; min-height: 13px;" class=""><br class=""></div><div style="margin: 0px;" class="">int foo(int a) {</div><div style="margin: 0px;" class=""> int b = (int16_t)a + 8;</div><div style="margin: 0px;" class=""> int c = (int16_t)b + 8;</div><div style="margin: 0px;" class=""> int d = (int16_t)c + 8;</div><div style="margin: 0px;" class=""> int e = (int16_t)d + 8;</div><div style="margin: 0px;" class=""> int f = (int16_t)e + 8;</div><div style="margin: 0px;" class=""> return f;</div><div style="margin: 0px;" class="">}</div><div class=""><br class=""></div><div class="">I’d then do:</div><div class=""><div style="margin: 0px;" class="">$ ~/llvm/build_ninja/bin/opt -mem2reg repro.ll -S -o repro.opt.ll</div></div><div class=""><br class=""></div><div class="">to get the real test file, which crashes the compiler (with the assert but without the fix obviously) when doing:</div><div class=""><br class=""></div><div class="">$ ~/llvm/build_ninja/bin/llc repro.opt.ll -O2</div><div class=""><br class=""></div><div class="">(not sure if the -O2 is necessary)</div><div class="">The thing that makes this very prone to get to a not-testing-anything-at-all state is that the combiner is required not only to combine nodes (this gives you node deletion), but also to create nodes (this is the purpose of the narrowing casts, they’ll enable code paths where the combiner tries various widening options). If you remove one of the additions in the chain, then the assert doesn’t trigger because the memory doesn’t get reused by the “right” nodes.</div><div class=""><br class=""></div><div class="">WDYT?</div><div class=""><br class=""></div><div class="">Fred</div><div class=""> </div></div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class="">- David<br class=""> </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;"><div style="word-wrap: break-word;" class=""><div class=""><div class=""></div><div class=""><br class=""></div><div class="">Fred</div><div class=""><div class="h5"><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;"><div class="">- David<br class=""> </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;"><div style="word-wrap: break-word;" class=""><div class=""><div class=""><br class=""></div><div class="">Fred</div><div class=""><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><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;"><br class="">Modified:<br class=""> <span class=""> </span>llvm/trunk/include/llvm/CodeGen/SelectionDAG.h<br class=""> <span class=""> </span>llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp<br class=""><br class="">Modified: llvm/trunk/include/llvm/CodeGen/SelectionDAG.h<br class="">URL:<span class=""> </span><a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/SelectionDAG.h?rev=221709&r1=221708&r2=221709&view=diff" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/SelectionDAG.h?rev=221709&r1=221708&r2=221709&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/include/llvm/CodeGen/SelectionDAG.h (original)<br class="">+++ llvm/trunk/include/llvm/CodeGen/SelectionDAG.h Tue Nov 11 15:21:08 2014<br class="">@@ -128,6 +128,10 @@ public:<br class=""> DbgValMap[Node].push_back(V);<br class=""> }<br class=""><br class="">+ /// \brief Invalidate all DbgValues attached to the node and remove<br class="">+ /// it from the Node-to-DbgValues map.<br class="">+ void erase(const SDNode *Node);<br class="">+<br class=""> void clear() {<br class=""> DbgValMap.clear();<br class=""> DbgValues.clear();<br class=""><br class="">Modified: llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp<br class="">URL:<span class=""> </span><a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp?rev=221709&r1=221708&r2=221709&view=diff" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp?rev=221709&r1=221708&r2=221709&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (original)<br class="">+++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp Tue Nov 11 15:21:08 2014<br class="">@@ -687,6 +687,15 @@ void SelectionDAG::DeleteNodeNotInCSEMap<br class=""> DeallocateNode(N);<br class=""> }<br class=""><br class="">+void SDDbgInfo::erase(const SDNode *Node) {<br class="">+ DbgValMapType::iterator I = DbgValMap.find(Node);<br class="">+ if (I == DbgValMap.end())<br class="">+ return;<br class="">+ for (auto &Val: I->second)<br class="">+ Val->setIsInvalidated();<br class="">+ DbgValMap.erase(I);<br class="">+}<br class="">+<br class=""> void SelectionDAG::DeallocateNode(SDNode *N) {<br class=""> if (N->OperandsNeedDelete)<br class=""> delete[] N->OperandList;<br class="">@@ -697,10 +706,9 @@ void SelectionDAG::DeallocateNode(SDNode<br class=""><br class=""> NodeAllocator.Deallocate(AllNodes.remove(N));<br class=""><br class="">- // If any of the SDDbgValue nodes refer to this SDNode, invalidate them.<br class="">- ArrayRef<SDDbgValue*> DbgVals = DbgInfo->getSDDbgValues(N);<br class="">- for (unsigned i = 0, e = DbgVals.size(); i != e; ++i)<br class="">- DbgVals[i]->setIsInvalidated();<br class="">+ // If any of the SDDbgValue nodes refer to this SDNode, invalidate<br class="">+ // them and forget about that node.<br class="">+ DbgInfo->erase(N);<br class=""> }<br class=""><br class=""> #ifndef NDEBUG<br class=""><br class=""><br class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank" class="">llvm-commits@cs.uiuc.edu</a><br class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></blockquote></div></div></div></div></blockquote></div></div></div></div></blockquote></div></div></blockquote></div></div></div></div></blockquote></div></div></blockquote></div><br class=""></body></html>