[PATCH] Propagate DAG node ordering during legalization and instruction selection

[Justin Holewinski]

On Wed, Mar 20, 2013 at 3:01 AM, Andrew Trick <atrick at apple.com> wrote:

>
> On Mar 19, 2013, at 1:17 PM, Justin Holewinski <
> justin.holewinski at gmail.com> wrote:
>
> Updated patch attached.
>
> I've addressed the CSE during legalization issue.  Now ordering is only
> propagated if the new node does not have an ordering (is zero), or has an
> ordering that is greater than the replaced node.
>
> As for compile time, I think I may have had some other machine
> interference in the 8% figure.  I can't reproduce that now, and both LLVM
> unit tests and LNT are not showing any statistically significant
> differences.  I see some variation across runs in LNT, but it looks to be
> machine noise as I see both regressions and improvements in different
> benchmarks in different runs.  I see +/- 0.5% in the unit tests, but that
> goes both ways.  Both tests use release+asserts build.
>
>
> Your patch looks fine but doesn't go far enough. I'd like to add an
> IROrder field to SDNode (eventually we might be able to make it redundant
> with NodeId, although there would be temporary points at which nodes have
> to share an IROrder). That would remove any concerns about the compile-time
> of potentially frequent DenseMap lookup. But I really want to do it to help
> ensure that IROrder remains present and valid as a topological order. Then
> we don't need a "source order" scheduler at all, which would be really
> great. Just emit MIs for the selected nodes in place, and break some
> physreg interferences.
>
> Ideally, anyone who creates an SDNode needs to track down the
> IROrder. Rather than propagating IROrder when we replace all uses, we would
> do it when we morph or CSE the node, similar to DebugLoc. Ensuring
> topological order is another aspect of the problem that can be dealt with
> later.
>
> It's a big infrastructure project. But if you find any part of this plan
> will help you, progress toward that goal is welcome.
>

That makes perfect sense to me.  I'm not sure I can justify the time
commitment to implement that soon, but it's certainly something to keep in
mind.  With any luck, I won't need to rely on source-level scheduling for
too much longer!

Out of curiosity, what was the design rationale for keeping the ordering
separate from SDNode in the first place?


>
> -Andy
>
> On Tue, Mar 19, 2013 at 1:07 PM, Justin Holewinski <
> justin.holewinski at gmail.com> wrote:
>
>> On Tue, Mar 19, 2013 at 12:47 PM, Justin Holewinski <
>> justin.holewinski at gmail.com> wrote:
>>
>>>
>>> On Tue, Mar 19, 2013 at 2:26 AM, Evan Cheng <evan.cheng at apple.com>wrote:
>>>
>>>>
>>>>
>>>> Sent from my iPad
>>>>
>>>> On Mar 18, 2013, at 2:02 PM, Justin Holewinski <
>>>> justin.holewinski at gmail.com> wrote:
>>>>
>>>> Compile-time impact is negligible for a release build on the unit
>>>> tests.  There is about an 8% impact with assertions enabled.
>>>>
>>>>
>>>> Unit tests are much too small for measuring compile time. 8% for
>>>> assertion build is massive. Why are there such large discrepancy?
>>>>
>>>
>>> I've been trying to get measurements from LNT, but I'm getting too much
>>> run-to-run variation.  A few benchmarks show significant changes (both
>>> positive and negative), but the affected benchmarks are different from run
>>> to run.  The vast majority of benchmarks show no change.  Do you have any
>>> suggestions for getting concrete compile time data?
>>>
>>
>> I should also note that the 8% was only on the LLVM tests, where isel is
>> a large chunk of the testing time.  Including the clang tests, the
>> difference is around 0.5% (with run-to-run variation).
>>
>>
>>>
>>>
>>>>
>>>> Evan
>>>>
>>>>
>>>> I'm not sure how CSE factors in here.  This propagation happens during
>>>> legalization.  I agree that it should be addressed, but I see it as a
>>>> separate issue.  Unless I'm missing some instance of CSE being performed
>>>> during legalization?
>>>>
>>>>
>>>> On Mon, Mar 18, 2013 at 4:04 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk
>>>> > wrote:
>>>>
>>>>>
>>>>> On Mar 15, 2013, at 10:51 AM, Justin Holewinski <
>>>>> justin.holewinski at gmail.com> wrote:
>>>>>
>>>>> Thanks Jakob!
>>>>>
>>>>> Modified patch attached.  Anyone adverse to this change?
>>>>>
>>>>>
>>>>> +
>>>>> +  // Propagate ordering
>>>>> +  unsigned Order = DAG.GetOrdering(Op.getNode());
>>>>> +  DAG.AssignOrdering(Lo.getNode(), Order);
>>>>> +  DAG.AssignOrdering(Hi.getNode(), Order);
>>>>>
>>>>> Did you consider what happens when CSE kicks in? The new nodes may not
>>>>> be new, they could have been CSE'ed with an existing expression.
>>>>>
>>>>> When that happens, the ordering should be the earlier of the two.
>>>>>
>>>>> I think it would make sense to teach the SDNodeOrdering class about
>>>>> this.
>>>>>
>>>>> /jakob
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>>
>>>> Thanks,
>>>>
>>>> Justin Holewinski
>>>>
>>>> _______________________________________________
>>>>
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>>
>>>
>>>
>>> --
>>>
>>> Thanks,
>>>
>>> Justin Holewinski
>>>
>>
>>
>>
>> --
>>
>> Thanks,
>>
>> Justin Holewinski
>>
>
>
>
> --
>
> Thanks,
>
> Justin Holewinski
>  <0001-Propagate-DAG-node-ordering-during-type-legalization.patch>
> _______________________________________________
>
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>


-- 

Thanks,

Justin Holewinski
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130320/e562be13/attachment.html>