bug in PrescheduleNodesWithMultipleUses()

[Andrew Trick]

On Sep 6, 2013, at 4:29 AM, Robert Lytton <robert at xmos.com> wrote:

> Hi Andrew,
> 
> Changing to Sched::Source seems fine.
> Thank you.
> Patch attached.
> 
> Thank you also for the answers to my questions - appreciated.
> 
> I notice that only the X86 target uses resetOperationActions(), which in turn is called by the X86TargetLowering() constructor
> All other targets (including XCore) do all the work in the <target>TargetLowering() constructors.
> Should anything that does not require the '<target>TargetMachine' argument be moved into resetOperationActions()?
> Is this the only intended use of resetOperationActions() - to be called during the <target>TargetLowering() constructor viz:
>         // TargetLowering Configuration Methods - These methods should be invoked by
>         // the derived class constructor to configure this object for the target.
> 
> Robert
> 

Hi Robert,

Sorry I didn’t respond. I’m glad you committed this. There is a type in the test filename “shedule”.

resetOperationActions reruns for each machine function. Function attributes can change, including setting a different subtarget. It’s probably not something you would notice, but seems like good practice to move things into resetOperationActions. I have no idea why x86 sets these in the ctor:

  X86ScalarSSEf64 = Subtarget->hasSSE2();
  X86ScalarSSEf32 = Subtarget->hasSSE1();

Must be some strange initialization order thing...

-Andy

> 
> From: Andrew Trick [atrick at apple.com]
> Sent: 04 September 2013 06:36
> To: Robert Lytton
> Cc: llvm commits
> Subject: Re: bug in PrescheduleNodesWithMultipleUses()
> 
> 
> 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
> 
> <PatchScheduler>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130919/50557619/attachment.html>