[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 chandlerc at gmail.com
Wed Jul 23 04:12:08 PDT 2014

And as Joey reminded me, I should clarify why I just nuked the aarch64
test: the test is useless. It CHECKs for an 'ldr' instruction.... which is
loading into the zero register! Unsurprisingly, when this instruction gets
run through the combiner, it gets removed.

On Wed, Jul 23, 2014 at 4:09 AM, Chandler Carruth <chandlerc at gmail.com>

> 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/cb665ee2/attachment.html>

More information about the llvm-commits mailing list