bug in PrescheduleNodesWithMultipleUses()
Andrew Trick
atrick at apple.com
Tue Sep 3 22:36:34 PDT 2013
On Aug 23, 2013, at 3:11 AM, Robert Lytton <robert at xmos.com> wrote:
> (this picks up from the thread in llvmdev: "PrescheduleNodesWithMultipleUses() causing failure in PickNodeToScheduleBottomUp() ???")
>
> PrescheduleNodesWithMultipleUses() is a function that tries to transform a DAG where nodes have multiple uses, with the aim of reducing the live range.
> (see comment in lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp)
>
> However, it does not check to see if callResources will be affected by the transformation.
>
>
> The problem
> ========
> Take the following correctly scheduled DAG (arrow to predecessor):
>
> SetUp2 SetUp1
> ^ ^
> | |
> | |
> Destroy2---->PredSU <----SU
> ^ ^ ^
> | | |
> | | |
> ----------- | ---------
> | | |
> Destroy1
> ^
> |
>
> In this example there are two successors of 'PredSU' with type
> getCallFrameDestroyOpcode (Destroy) and one is a successor of the other.
> Taking the successor of the two Destroys (Destroy1), note that it's
> matching getCallFrameSetupOpcode (Setup1) is a predecessor of 'SU'.
> In this situation, re-routing the dependency on 'PredSU' through 'SU' will
> cause a dead lock Viz:
>
> SetUp2 PredSU SetUp1
> ^ ^ ^
> | | |
> | | |
> Destroy2-----> SU -------
> ^ ^ ^
> | | |
> | | |
> ----------- | |
> | | |
> Destroy1
> ^
> |
>
> The new function callResourceDeadLockDanger() will check for this situation
> and prevent PrescheduleNodesWithMultipleUses() from making the transformation
> if it will cause a callResource dead lock.
Robert,
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.
All targets should move to source order SD scheduling:
<target>TargetLowering::resetOperationActions() {
...
setSchedulingPreference(Sched::Source);
If instruction scheduling is required, the target can enable MachineScheduler:
bool <target>SubTargetInfo::enableMachineScheduler() const { return true; }
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:
setSchedulingPreference(Sched::ILP);
setSchedulingPreference(Sched::Hybrid);
For educational purposes I'll try to answer your questions below (although the only good fix I can see is deleting PrescheduleNodesWithMultipleUses()).
> Outstanding issues
> ============
>
> 1. Is it too aggressive in searching predecessors and successors?
> Should the algorithm give up and assume the worst if the depth of search reaches a predefined limit?
> Can someone confirm that the extra processing time is not onerous.
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.
I also think recursive DAG walking is very bad form. LLVM runtime compilers don’t want to see stack overflow.
> 2. Should the initial search for 'SetUp1' and 'Destroy1' only search along chains? viz conditional upon II->isCtrl()
> This will reduce the search space, but are getCallFrameSetupOpcode & getCallFrameDestroyOpcode always 'chained'?
> (Later searches for Destroy2 need to check all predecessors)
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.
> 3. What is the best way to construct the test case?
> Using an IR as input does not guarantee the required DAG will be output for testing.
> The test IR only produces the correct DAG when built for the XCore target.
> (see attached test case)
That’s because other targets don’t call PrescheduleNodesWithMultipleUses.
> 4. Where should the test be placed?
> Currently I have placed it under test/CodeGen/XCore but it relates to lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
That’s fine. Generic test cases are better unless you need to CHECK the disassembly.
-Andy
>
> Thank you
>
> Robert
>
> <PatchCallResourceDeadLock>_______________________________________________
> 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/20130903/e70769a1/attachment.html>
More information about the llvm-commits
mailing list