<html dir="ltr">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
<style id="owaParaStyle" type="text/css">P {margin-top:0;margin-bottom:0;}</style>
</head>
<body ocsi="0" fpstyle="1" style="word-wrap:break-word">
<div style="direction: ltr;font-family: Tahoma;color: #000000;font-size: 10pt;">Hi Andrew,<br>
<br>
Changing to Sched::Source seems fine.<br>
Thank you.<br>
Patch attached.<br>
<br>
Thank you also for the answers to my questions - appreciated.<br>
<br>
I notice that only the X86 target uses resetOperationActions(), which in turn is called by the X86TargetLowering() constructor<br>
All other targets (including XCore) do all the work in the <target>TargetLowering() constructors.<br>
Should anything that does not require the '<target>TargetMachine' argument be moved into resetOperationActions()?<br>
Is this the only intended use of resetOperationActions() - to be called during the <target>TargetLowering() constructor viz:<br>
// TargetLowering Configuration Methods - These methods should be invoked by<br>
// the derived class constructor to configure this object for the target.<br>
<br>
Robert<br>
<br>
<br>
<div style="font-family: Times New Roman; color: #000000; font-size: 16px">
<hr tabindex="-1">
<div style="direction: ltr;" id="divRpF929133"><font color="#000000" face="Tahoma" size="2"><b>From:</b> Andrew Trick [atrick@apple.com]<br>
<b>Sent:</b> 04 September 2013 06:36<br>
<b>To:</b> Robert Lytton<br>
<b>Cc:</b> llvm commits<br>
<b>Subject:</b> Re: bug in PrescheduleNodesWithMultipleUses()<br>
</font><br>
</div>
<div></div>
<div><br>
<div>
<div>On Aug 23, 2013, at 3:11 AM, Robert Lytton <<a href="mailto:robert@xmos.com" target="_blank">robert@xmos.com</a>> wrote:</div>
<blockquote type="cite">
<div style="font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:auto; text-align:start; text-indent:0px; text-transform:none; white-space:normal; widows:auto; word-spacing:0px; direction:ltr; font-family:Tahoma; font-size:10pt">
<div style="font-family:'Times New Roman'; font-size:16px">
<div style="direction:ltr; font-family:Tahoma; font-size:10pt">(this picks up from the thread in llvmdev: "PrescheduleNodesWithMultipleUses() causing failure in PickNodeToScheduleBottomUp() ???")<br>
<br>
PrescheduleNodesWithMultipleUses() is a function that tries to transform a DAG where nodes have multiple uses, with the aim of reducing the live range.<br>
(see comment in lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp)<br>
<br>
However, it does not check to see if callResources will be affected by the transformation.<br>
<br>
<br>
The problem<br>
========<br>
Take the following correctly scheduled DAG (arrow to predecessor):<br>
<font face="Courier New"><br>
SetUp2 SetUp1<br>
^ ^<br>
| |<br>
| |<br>
Destroy2---->PredSU <----SU<br>
^ ^ ^<br>
| | |<br>
| | |<br>
----------- | ---------<br>
| | |<br>
Destroy1<br>
^<br>
|<br>
</font><br>
In this example there are two successors of 'PredSU' with type<br>
getCallFrameDestroyOpcode (Destroy) and one is a successor of the other.<br>
Taking the successor of the two Destroys (Destroy1), note that it's<br>
matching getCallFrameSetupOpcode (Setup1) is a predecessor of 'SU'.<br>
In this situation, re-routing the dependency on 'PredSU' through 'SU' will<br>
cause a dead lock Viz:<br>
<br>
<font face="Courier New"> SetUp2 PredSU SetUp1<br>
^ ^ ^<br>
| | |<br>
| | |<br>
Destroy2-----> SU -------<br>
^ ^ ^<br>
| | |<br>
| | |<br>
----------- | |<br>
| | |<br>
Destroy1<br>
^<br>
|<br>
</font><br>
<font face="Tahoma" size="2">The new function callResourceDeadLockDanger()</font><span class="Apple-converted-space"> </span>will check for this situation<br>
and prevent PrescheduleNodesWithMultipleUses() from making the transformation<br>
if it will cause a callResource dead lock.<br>
</div>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>Robert,</div>
<div><br>
</div>
<div>I appreciate that you put a lot of care into this patch. Your comments are amazing. But do you absolutely need to call PrescheduleNodesWithMultipleUses? x86_64 and arm don't use it, and it is about to be deprecated.</div>
<div><br>
</div>
<div>All targets should move to source order SD scheduling:</div>
<div><br>
</div>
<div><target>TargetLowering::resetOperationActions() {</div>
<div>...</div>
<div> setSchedulingPreference(Sched::Source);</div>
<div><br>
</div>
<div>If instruction scheduling is required, the target can enable MachineScheduler:</div>
<div><br>
</div>
<div>bool <target>SubTargetInfo::enableMachineScheduler() const { return true; }</div>
<div><br>
</div>
<div>If for some reason the source order scheduler won't work in the short term, you can choose one of these SD schedulers that don't run PrescheduleNodesWithMultipleUses:</div>
<div><br>
</div>
<div>setSchedulingPreference(Sched::ILP);</div>
<div>setSchedulingPreference(Sched::Hybrid);</div>
<div><br>
</div>
<div>For educational purposes I'll try to answer your questions below (although the only good fix I can see is deleting PrescheduleNodesWithMultipleUses()).</div>
<div><br>
</div>
<blockquote type="cite">
<div style="font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:auto; text-align:start; text-indent:0px; text-transform:none; white-space:normal; widows:auto; word-spacing:0px; direction:ltr; font-family:Tahoma; font-size:10pt">
<div style="font-family:'Times New Roman'; font-size:16px">
<div style="direction:ltr; font-family:Tahoma; font-size:10pt">Outstanding issues<br>
============<br>
<br>
1. Is it too aggressive in searching predecessors and successors?<br>
Should the algorithm give up and assume the worst if the depth of search reaches a predefined limit?<br>
Can someone confirm that the extra processing time is not onerous.<br>
</div>
</div>
</div>
</blockquote>
<div><br>
</div>
Walking all paths in the DAG is exponential. In a case like this, a search limit is good. Otherwise you need to maintain a visited set.</div>
<div><br>
</div>
<div>I also think recursive DAG walking is very bad form. LLVM runtime compilers don’t want to see stack overflow.</div>
<div><br>
<blockquote type="cite">
<div style="font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:auto; text-align:start; text-indent:0px; text-transform:none; white-space:normal; widows:auto; word-spacing:0px; direction:ltr; font-family:Tahoma; font-size:10pt">
<div style="font-family:'Times New Roman'; font-size:16px">
<div style="direction:ltr; font-family:Tahoma; font-size:10pt">2. Should the initial search for 'SetUp1' and 'Destroy1' only search along chains? viz conditional upon II->isCtrl()<br>
This will reduce the search space, but are getCallFrameSetupOpcode & getCallFrameDestroyOpcode always 'chained'?<br>
(Later searches for Destroy2 need to check all predecessors)<br>
</div>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>Yes, the call sequence nodes are chained. Not all nodes within the sequence are chained. If a node in the call sequence has a chain edge, Setup/Destroy will be reachable in that direction.</div>
<br>
<blockquote type="cite">
<div style="font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:auto; text-align:start; text-indent:0px; text-transform:none; white-space:normal; widows:auto; word-spacing:0px; direction:ltr; font-family:Tahoma; font-size:10pt">
<div style="font-family:'Times New Roman'; font-size:16px">
<div style="direction:ltr; font-family:Tahoma; font-size:10pt">3. What is the best way to construct the test case?<br>
Using an IR as input does not guarantee the required DAG will be output for testing.<br>
The test IR only produces the correct DAG when built for the XCore target.<br>
(see attached test case)<br>
</div>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>That’s because other targets don’t call PrescheduleNodesWithMultipleUses.</div>
<div><br>
</div>
<blockquote type="cite">
<div style="font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:auto; text-align:start; text-indent:0px; text-transform:none; white-space:normal; widows:auto; word-spacing:0px; direction:ltr; font-family:Tahoma; font-size:10pt">
<div style="font-family:'Times New Roman'; font-size:16px">
<div style="direction:ltr; font-family:Tahoma; font-size:10pt">4. Where should the test be placed?<br>
Currently I have placed it under test/CodeGen/XCore but it relates to lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp<br>
</div>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>That’s fine. Generic test cases are better unless you need to CHECK the disassembly.</div>
<div><br>
</div>
<div>-Andy</div>
<br>
<blockquote type="cite">
<div style="font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:auto; text-align:start; text-indent:0px; text-transform:none; white-space:normal; widows:auto; word-spacing:0px; direction:ltr; font-family:Tahoma; font-size:10pt">
<div style="font-family:'Times New Roman'; font-size:16px">
<div style="direction:ltr; font-family:Tahoma; font-size:10pt"><br>
Thank you<br>
<br>
Robert<br>
<br>
</div>
</div>
</div>
<span><PatchCallResourceDeadLock></span><span style="font-family:Helvetica; font-size:12px; font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:auto; text-align:start; text-indent:0px; text-transform:none; white-space:normal; widows:auto; word-spacing:0px; float:none; display:inline!important">_______________________________________________</span><br style="font-family:Helvetica; font-size:12px; font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:auto; text-align:start; text-indent:0px; text-transform:none; white-space:normal; widows:auto; word-spacing:0px">
<span style="font-family:Helvetica; font-size:12px; font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:auto; text-align:start; text-indent:0px; text-transform:none; white-space:normal; widows:auto; word-spacing:0px; float:none; display:inline!important">llvm-commits
mailing list</span><br style="font-family:Helvetica; font-size:12px; font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:auto; text-align:start; text-indent:0px; text-transform:none; white-space:normal; widows:auto; word-spacing:0px">
<a href="mailto:llvm-commits@cs.uiuc.edu" style="font-family:Helvetica; font-size:12px; font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:auto; text-align:start; text-indent:0px; text-transform:none; white-space:normal; widows:auto; word-spacing:0px" target="_blank">llvm-commits@cs.uiuc.edu</a><br style="font-family:Helvetica; font-size:12px; font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:auto; text-align:start; text-indent:0px; text-transform:none; white-space:normal; widows:auto; word-spacing:0px">
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" style="font-family:Helvetica; font-size:12px; font-style:normal; font-variant:normal; font-weight:normal; letter-spacing:normal; line-height:normal; orphans:auto; text-align:start; text-indent:0px; text-transform:none; white-space:normal; widows:auto; word-spacing:0px" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></blockquote>
</div>
<br>
</div>
</div>
</div>
</body>
</html>