[llvm] Preferential treatment of labels in MI sched DAG construction

[Andrew Trick]

On Feb 11, 2013, at 1:50 PM, Sergei Larin <slarin at codeaurora.org> wrote:

> 
> Andy,
> 
> 
>  Following our discussion below here is the proposed patch.
> I also looked at the comment language for the CanHandleTerminators:
> 
>    /// The standard DAG builder does not normally include terminators as
> DAG
>    /// nodes because it does not create the necessary dependencies to
> prevent
>    /// reordering. A specialized scheduler can overide
>    /// TargetInstrInfo::isSchedulingBoundary then enable this flag to
> indicate
>    /// it has taken responsibility for scheduling the terminator correctly.
>    bool CanHandleTerminators;
> 
> ...and it seems clear and all encompassing enough to cover the proposed
> change. If you would like to modify it, just let me know and I'll amend the
> patch. Thanks.

Hi Sergei,

I might have misunderstood, but to make the requirements explicit for targets that do not have custom terminator/label schedulers, I think we should have this:

-    assert((!MI->isTerminator() || CanHandleTerminators) && !MI->isLabel() &&
+    assert(CanHandleTerminators || (!MI->isTerminator() && MI->isLabel()) &&

-Andy

> ---
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
> The Linux Foundation
> 
> 
>> -----Original Message-----
>> From: Andrew Trick [mailto:atrick at apple.com]
>> Sent: Monday, February 11, 2013 3:23 PM
>> To: Sergei Larin
>> Cc: llvmdev at cs.uiuc.edu
>> Subject: Re: [LLVMdev] Preferential treatment of labels in MI sched DAG
>> construction
>> 
>> 
>> On Feb 11, 2013, at 1:03 PM, Sergei Larin <slarin at codeaurora.org>
>> wrote:
>> 
>>> Hi Andy,
>>> 
>>> I have to resurrect an ancient question regarding scheduling
>> boundaries.
>>> 
>>> You might remember the reason for introduction of
>> CanHandleTerminators
>>> to ScheduleDAGInstrs. In short, Hexagon is currently uses DAG
>>> construction method (buildSchedGraph) for several purposes, which
>>> includes region formation for general VLIW packetization/bundling. As
>>> such we need to handle pretty much all instructions and any
>>> terminators, including labels (though I know it sounds strange).
>>> Nevertheless, ScheduleDAGInstrs::buildSchedGraph
>>> still has this hard assert in it:
>>> 
>>> assert((!MI->isTerminator() || CanHandleTerminators) && !MI-
>>> isLabel() &&
>>>          "Cannot schedule terminators or labels!");
>>> 
>>> ...and we have been OK till now by simply disabling it in our local
>>> version of the code, but since we are now working towards upstreaming
>>> all our code, this could no longer be ignored.
>>> 
>>> I guess my proposal is not to treat labels differently from any
>> other
>>> terminator - if a target wants to "schedule" labels - let it do so.
>> It
>>> actually works just fine for Hexagon... so if we can change this
>>> assert to something like:
>>> 
>>> 
>>> assert((!MI->isTerminator() || CanHandleTerminators) && "Cannot
>>> schedule terminators or labels!");
>>> 
>>> It would equal label in rights with any other terminator. What do
>> you
>>> think?
>> 
>> Sure, as long as CanHandleTerminators is commented to that effect.
>> 
>> -Andy
> <CanHandleTerminators.patch>