<p dir="ltr">Cc'ing Bill Wendling. This should be in 3.5 because at least on (open)SUSE we bootstrap clang on PPC64.<br>
</p>
<div class="gmail_quote">On Aug 15, 2014 4:59 AM, "Bill Schmidt" <<a href="mailto:wschmidt@linux.vnet.ibm.com">wschmidt@linux.vnet.ibm.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: wschmidt<br>
Date: Thu Aug 14 20:25:26 2014<br>
New Revision: 215685<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=215685&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=215685&view=rev</a><br>
Log:<br>
[PPC64] Add missing dependency on X2 to LDinto_toc.<br>
<br>
The LDinto_toc pattern has been part of 64-bit PowerPC for a long<br>
time, and represents loading from a memory location into the TOC<br>
register (X2). However, this pattern doesn't explicitly record that<br>
it modifies that register. This patch adds the missing dependency.<br>
<br>
It was very surprising to me that this has never shown up as a problem<br>
in the past, and that we only saw this problem recently in a single<br>
scenario when building a self-hosted clang. It turns out that in most<br>
cases we have another dependency present that keeps the LDinto_toc<br>
instruction tied in place. LDinto_toc is used for TOC restore<br>
following a call site, so this is a typical sequence:<br>
<br>
BCTRL8 <regmask>, %CTR8<imp-use>, %RM<imp-use>, %X3<imp-use>, %X12<imp-use>, %X1<imp-def>, ...<br>
LDinto_toc 24, %X1<br>
ADJCALLSTACKUP 96, 0, %R1<imp-def>, %R1<imp-use><br>
<br>
Because the LDinto_toc is inserted prior to the ADJCALLSTACKUP, there<br>
is a natural anti-dependency between the two that keeps it in place.<br>
<br>
Therefore we don't usually see a problem. However, in one particular<br>
case, one call is followed immediately by another call, and the second<br>
call requires a parameter that is a TOC-relative address. This is the<br>
code sequence:<br>
<br>
BCTRL8 <regmask>, %CTR8<imp-use>, %RM<imp-use>, %X3<imp-use>, %X4<imp-use>, %X5<imp-use>, %X12<imp-use>, %X1<imp-def>, ...<br>
LDinto_toc 24, %X1<br>
ADJCALLSTACKUP 96, 0, %R1<imp-def>, %R1<imp-use><br>
ADJCALLSTACKDOWN 96, %R1<imp-def>, %R1<imp-use><br>
%vreg39<def> = ADDIStocHA %X2, <ga:@.str>; G8RC_and_G8RC_NOX0:%vreg39<br>
%vreg40<def> = ADDItocL %vreg39<kill>, <ga:@.str>; G8RC:%vreg40 G8RC_and_G8RC_NOX0:%vreg39<br>
<br>
Note that the back-to-back stack adjustments are the same size! The<br>
back end is smart enough to recognize this and optimize them away:<br>
<br>
BCTRL8 <regmask>, %CTR8<imp-use>, %RM<imp-use>, %X3<imp-use>, %X4<imp-use>, %X5<imp-use>, %X12<imp-use>, %X1<imp-def>, ...<br>
LDinto_toc 24, %X1<br>
%vreg39<def> = ADDIStocHA %X2, <ga:@.str>; G8RC_and_G8RC_NOX0:%vreg39<br>
%vreg40<def> = ADDItocL %vreg39<kill>, <ga:@.str>; G8RC:%vreg40 G8RC_and_G8RC_NOX0:%vreg39<br>
<br>
Now there is nothing to prevent the ADDIStocHA instruction from moving<br>
ahead of the LDinto_toc instruction, and because of the longest-path<br>
heuristic, this is what happens.<br>
<br>
With the accompanying patch, %X2 is represented as an implicit def:<br>
<br>
BCTRL8 <regmask>, %CTR8<imp-use>, %RM<imp-use>, %X3<imp-use>, %X4<imp-use>, %X5<imp-use>, %X12<imp-use>, %X1<imp-def>, ...<br>
LDinto_toc 24, %X1, %X2<imp-def,dead><br>
ADJCALLSTACKUP 96, 0, %R1<imp-def,dead>, %R1<imp-use><br>
ADJCALLSTACKDOWN 96, %R1<imp-def,dead>, %R1<imp-use><br>
%vreg39<def> = ADDIStocHA %X2, <ga:@.str>; G8RC_and_G8RC_NOX0:%vreg39<br>
%vreg40<def> = ADDItocL %vreg39<kill>, <ga:@.str>; G8RC:%vreg40 G8RC_and_G8RC_NOX0:%vreg39<br>
<br>
So now when the two stack adjustments are removed, ADDIStocHA is<br>
prevented from being moved above LDinto_toc.<br>
<br>
I have not yet created a test case for this, because the original<br>
failure occurs on a relatively large function that needs reduction.<br>
However, this is a fairly serious bug, despite its infrequency, and I<br>
wanted to get this patch onto the list as soon as possible so that it<br>
can be considered for a 3.5 backport. I'll work on whittling down a<br>
test case.<br>
<br>
Have we missed the boat for 3.5 at this point?<br>
<br>
Thanks,<br>
Bill<br>
<br>
Modified:<br>
llvm/trunk/lib/Target/PowerPC/PPCInstr64Bit.td<br>
<br>
Modified: llvm/trunk/lib/Target/PowerPC/PPCInstr64Bit.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCInstr64Bit.td?rev=215685&r1=215684&r2=215685&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCInstr64Bit.td?rev=215685&r1=215684&r2=215685&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Target/PowerPC/PPCInstr64Bit.td (original)<br>
+++ llvm/trunk/lib/Target/PowerPC/PPCInstr64Bit.td Thu Aug 14 20:25:26 2014<br>
@@ -802,7 +802,7 @@ def LDtocCPT: Pseudo<(outs g8rc:$rD), (i<br>
[(set i64:$rD,<br>
(PPCtoc_entry tconstpool:$disp, i64:$reg))]>, isPPC64;<br>
<br>
-let hasSideEffects = 1, isCodeGenOnly = 1, RST = 2 in<br>
+let hasSideEffects = 1, isCodeGenOnly = 1, RST = 2, Defs = [X2] in<br>
def LDinto_toc: DSForm_1<58, 0, (outs), (ins memrix:$src),<br>
"ld 2, $src", IIC_LdStLD,<br>
[(PPCload_toc ixaddr:$src)]>, isPPC64;<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>