bug in PrescheduleNodesWithMultipleUses()

[Eric Christopher]

Adding Andy to the thread...

-eric

On Mon, Sep 2, 2013 at 6:19 AM, Robert Lytton <robert at xmos.com> wrote:
> Hi,
>
> I would like to commit this patch.
> Could someone give insight into the possible compilation-speed-optimization
> of only searching chains when traversing the DAG?
> Thank you
>
> Robert
>
> ________________________________
> From: llvm-commits-bounces at cs.uiuc.edu [llvm-commits-bounces at cs.uiuc.edu] on
> behalf of Robert Lytton [robert at xmos.com]
> Sent: 23 August 2013 11:11
> To: llvm-commits at cs.uiuc.edu
> Subject: bug in PrescheduleNodesWithMultipleUses()
>
>
> Hi
>
> (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.
>
>
> 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.
>
> 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)
>
> 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)
>
> 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
>
> Thank you
>
> Robert
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>