[PATCH] [SDAG] Introduce a combined set to the DAG combiner which tracks nodes which have successfully round-tripped through the combine phase, and use this to ensure all operands to DAG nodes are visited by the combiner, even if they are only added...

[Chandler Carruth]

A couple of random other notes about this patch....

- I did benchmark the compile time impact. For a "worst case" scenario in
my mind of taking a single bitcode for all of clang and running it through
just llc I observed a 3% slowdown of llc with this patch. I expect that to
translate into more like 1% or 2% of slowdown for LTO, and well under that
for normal compiles (which have the frontend in the critical path). So the
runtime cost isn't great, but isn't terribly scary to me.

- An obvious question is, why still populate the worklist with all the
nodes? The answer is because just starting from the root and letting the
de-dup'ed operand walk handle the rest causes more tests to fail, and I'm
already struggling to drag the test suite kicking and screaming through
these changes. It still might be nice eventually so that we move to a more
principled graph-traversal strategy. It may recover a (very minor) fraction
of the runtime cost, but nothing huge.

- With this functionality added, I can add legalization support to the last
DAG combine phase and successfully legalize newly created illegal nodes as
we effectively traverse the new nodes. I've successfully implemented the
PSHUFB matching logic on top of this. So this isn't just idle hardening of
the DAG combiner.


On Wed, Jul 23, 2014 at 3:58 AM, Chandler Carruth <chandlerc at gmail.com>
wrote:

> Hi grosbach, echristo, hfinkel, arsenm,
>
> ...during the combine phase.
>
> This is critical to have the combiner reach nodes that are *introduced*
> during combining. Previously these would sometimes be visited and
> sometimes not be visited based on whether they happened to end up on the
> worklist or not. Now we always run them through the combiner.
>
> This fixes quite a few bad codegen test cases lurking in the suite while
> also being more principled. Among these, the TLS codegeneration is
> particularly exciting for programs that have this in the critical path
> like TSan-instrumented binaries (although I think they engineer to use
> a different TLS that is faster anyways).
>
> However, there are also a worrying number of cases in x86 where this
> causes us to combine things into a DAG that misses the expected patterns
> and instruction selects to something pretty terrible. =/ The vector sign
> extension patterns in particular seem quite broken by this.
>
> Again, assistance would be appreciated from those familiar with other
> targets to evaluate the nature of these changes. I haven't even really
> vetted Mips or R600 for correctness. That's the primary reason for
> needing pre-commit review here.
>
> If anyone wants to take a look at the x86 regressions around sign
> extension and add some patterns or fix the combines to survive this
> change that would be amazingly helpful...
>
> http://reviews.llvm.org/D4638
>
> Files:
>   lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>   test/CodeGen/AArch64/arm64-dagcombiner-indexed-load.ll
>   test/CodeGen/ARM/aapcs-hfa-code.ll
>   test/CodeGen/Mips/cmov.ll
>   test/CodeGen/R600/add_i64.ll
>   test/CodeGen/R600/or.ll
>   test/CodeGen/X86/2010-04-23-mmx-movdq2q.ll
>   test/CodeGen/X86/avx-sext.ll
>   test/CodeGen/X86/i8-umulo.ll
>   test/CodeGen/X86/jump_sign.ll
>   test/CodeGen/X86/lower-bitcast.ll
>   test/CodeGen/X86/pr15267.ll
>   test/CodeGen/X86/store-narrow.ll
>   test/CodeGen/X86/trunc-ext-ld-st.ll
>   test/CodeGen/X86/vector-idiv.ll
>   test/CodeGen/X86/widen_cast-1.ll
>   test/CodeGen/X86/widen_conv-1.ll
>   test/CodeGen/X86/widen_load-2.ll
>   test/CodeGen/X86/x86-64-tls-1.ll
>   test/CodeGen/X86/x86-setcc-int-to-fp-combine.ll
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140723/424b1db5/attachment.html>