<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Chandler,<br>
<br>
Overall, this seems quite reasonable. My only minor comment would
be that you should improve the naming and documentation of
recursivelyDeleteUnusedNodes. It is not currently clear from the
interface that items are added to the worklist at all, or that it
is the frontier of used nodes which are added. Given this seems
to be important from your description, it should be documented. <br>
<br>
LGTM -- Mind you, I'm not particularly familiar with this code.
So take my LGTM with a grain or two of salt. :)<br>
<br>
Philip<br>
<br>
On 07/22/2014 03:45 AM, Chandler Carruth wrote:<br>
</div>
<blockquote
cite="mid:differential-rev-PHID-DREV-27qtxrd4h4mjm35zs7xu-req@reviews.llvm.org"
type="cite">
<pre wrap="">Hi hfinkel, arsenm, grosbach,
The old behavior could cause arbitrarily bad memory usage in the DAG
combiner if there was heavy traffic of adding nodes already on the
worklist to it. This commit switches the DAG combine worklist to work
the same way as the instcombine worklist where we null-out removed
entries and only add new entries to the worklist. This results in
subtle, frustrating churn in the particular order in which DAG combines
are applied which causes a number of minor regressions where we fail to
match a pattern previously matched by accident. AFAICT, all of these
should be using AddToWorklist to directly or should be written in a less
brittle way. None of the changes seem drastically bad, and a few of the
changes seem distinctly better.
A major change required to make this work is to significantly harden the
way in which the DAG combiner handle nodes which become dead
(zero-uses). Previously, we relied on the ability to "priority-bump"
them on the combine worklist to achieve recursive deletion of these
nodes and ensure that the frontier of remaining live nodes all were
added to the worklist. Instead, I've introduced a routine to just
implement that precise logic with no indirection. It is a significantly
simpler operation than that of the combiner worklist proper. I suspect
this will also fix some other problems with the combiner.
Note that I have *NO IDEA* what the changes on any architecture other than x86
really imply, please check these for your target! I just don't know how to
evaluate them. I've literally transcribed the test case changes necessary to
pass, but it may be more useful to patch in this change and compare A/B to
understand the differences for a particular test case.
I think the x86 changes are really minor and uninteresting, but the avx512 at
least is hiding a "regression" (but the test case is just noise, not testing
some performance invariant) that might be looked into. Not sure if any of the
others impact specific "important" code paths, but they didn't look terribly
interesting to me, or the changes were really minor.
However, maybe this entire approach is just deeply flawed? What do folks think,
is this worthwhile?
<a class="moz-txt-link-freetext" href="http://reviews.llvm.org/D4616">http://reviews.llvm.org/D4616</a>
Files:
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
test/CodeGen/ARM/fold-stack-adjust.ll
test/CodeGen/ARM/sxt_rot.ll
test/CodeGen/PowerPC/complex-return.ll
test/CodeGen/PowerPC/subsumes-pred-regs.ll
test/CodeGen/R600/r600-export-fix.ll
test/CodeGen/R600/swizzle-export.ll
test/CodeGen/Thumb2/thumb2-sxt_rot.ll
test/CodeGen/Thumb2/thumb2-uxt_rot.ll
test/CodeGen/X86/avx512-zext-load-crash.ll
test/CodeGen/X86/block-placement.ll
test/CodeGen/X86/divide-by-constant.ll
test/CodeGen/X86/fold-pcmpeqd-0.ll
test/CodeGen/X86/narrow-shl-load.ll
test/CodeGen/X86/store-narrow.ll
test/CodeGen/X86/vec_extract-sse4.ll
</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>
<a class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</body>
</html>