⚙ D21560 Relax the clearance calculating for breaking partial register dependency.
Relax the clearance calculating for breaking partial register dependency.
Authored by danielcdh on Jun 21 2016, 10:04 AM.
rG8cd84aaa6ff0: Relax the clearance calculating for breaking partial register dependency.
rL274068: Relax the clearance calculating for breaking partial register dependency.
LLVM assumes that large clearance will hide the partial register spill penalty. But in our experiment, 16 clearance is too small. As the inserted XOR is normally fairly cheap, we should have a higher clearance threshold to aggressively insert XORs that is necessary to break partial register dependency.
danielcdh retitled this revision from to Relax the clearance calculating for breaking partial register dependency..
danielcdh added reviewers: mkuper, davidxl, wmi.
danielcdh added a subscriber: llvm-commits.
Can you also add the original author who set the current threshold for the review?
This mostly makes sense to me. 16 is as much of a magic number as 64, so if this is supported by performance numbers, it's fine.
The problem is that we're using the number of instructions between the write and the read as a proxy for the "the latency of the critical chain between the write and the read". And it's not a very good proxy, since it means how correct we are depends on how much ILP the loop (assuming this is a loop-carried dependency) has. So any number we have here will be just hand-waving.
Adding some Intel people in case they have more input.
But fixing the threshold isn't good enough for several reasons.
(1) It isn't always possible, because the register may hold a live value. Marina (added) is working on a patch that will fix this problem.
(2) If the instruction has a "real" XMM operand, it is better to choose that register for the undef operand rather than inserting an xor. (Doing so hides the false dependence behind a true dependence that is unavoidable.) This applies to instructions like vcvtsd2ss et al. Marina is planning to work on this also.
Here is a testcase extracted from internal benchmark, which runs 2% faster on sandybridge if clearance threshold is changed from 16 to 64. For this extracted testcase itself, when threshold is set at 16, the cvt2si for "g" will not have xor inserted to break dependency. As a result, the inner loop consumes 13 cycles (comparing with 11 cycles if breaking the dependency for all "r", "g" and "b").
I don't have public benchmark result for this change yet. For internal benchmarks, no noticeable code size change has been observed. It has 2% speedup on the benchmark that motivated this patch, and has no performance impact on any other internal benchmarks.
It's great that Marina is working on a better fix for this problem. When is the ETA for the fix? If it's coming pretty soon, we should abandon this change. Otherwise if the change does no harm, let's just update the magic number so that we can reclaim performance for the motivating benchmark. Thoughts?
I think we should go ahead with this change. But I agree with davidxl that you should get an 'ok' from the original author, if possible.
Since even the code comments acknowledge that this is a repeated magic number, please give it a name. Even better would be to make it a cl::opt, so we can run experiments more easily.
FWIW, I don't see any measurable perf difference for the example test case (compiled with -O2) running on Haswell.
Is there already a regression test to show that we're not adding instructions if we're in MinSize mode?
I believe we currently add the xors in MinSize mode, and, as I wrote on PR22024, I think it's justified.
-Os: Like -O2 with extra optimizations to reduce code size.
-Oz: Like -Os (and thus -O2), but reduces code size further.
My gut feeling is that omitting the xor is not a good performance/size trade-off even for -Oz.
Yes, I pasted the links before I scrolled through the comments.
Is there already a regression test to show that we ARE adding instructions if we're in MinSize mode?
I don't have an explanation for that loss yet. The loop is optimally aligned on a 64-byte boundary. The extra xorps adds 3 bytes causing the inner loop to grow from an even 80 bytes to 83 bytes. The extra bytes require an additional ifetch operation that is somehow slowing down the whole chain?
Note that the partial update problem is limited to SSE codegen. If we use -mavx, we generate the 'v' versions of the conversion instructions. Those do not have partial reg update problems, and so we don't need any (v)xorps instructions in the loop. Performance for AVX versions of the program are slightly better than the best SSE case for both CPUs.
Put the magic number in an option.
The option is used for partial reg clearance and undef reg clearance. It should have a more generic name and description, or we should have 2 independent variables. I'm fine with either way, but what the patch has in this version is misleading.
Don't hard-code the comment to any specific value since it may change above.
Earlier in the thread, Joerg requested more general perf data. Has that request been fulfilled or cancelled?
It would be interesting to know what the optimal setting is for at least one benchmark, so you can leave a comment by the cl::opt definition to justify the default setting.
Also, we don't have to answer the MinSize question that I raised directly in this patch, but my perf data shows that we really shouldn't be adding xors in that situation - we hope these are free, but they are not in all cases. IMO, our documentation for -Oz is wrong. There shouldn't be any room for interpretation about the definition of "minimum"; this is the flag used by devs (embedded, boot code, etc) who require the smallest possible code, and perf shouldn't matter.
About the -Oz issue, I agree that there should be no xor inserted at -Oz. But that's a different issue and should be fixed in a separate patch.
That is a surprisingly large gain. This raises the question: if 64 is good, is 128 even better?
Agreed. Please let me know if you plan to follow-up on that part.
I have no further comments on this patch, so LGTM.