[llvm-commits] [PATCH] Allow SelectionDAGBuilder to reorder loads past stores

[Hal Finkel]

Andrew,

I've attached an updated patch. It includes a new ILP scheduling
heuristic that I found useful (on some x86_64 machines), and some other
improvements over the previous version.

Regarding the partial-aliasing problem you had pointed out, I don't
think that can be an problem here. The reason is that the patch
maintains the following invariant: all currently pending loads and
stores are commutable (exchangeable). There is still a critical chain,
but instead of consisting of all stores, the chain now consists of all
loads or stores that are not commutable with some previous load or
store. As a result, I do not think that it is possible to queue a load
or store that would cause an "aliasing cycle" such as in your example.

Please let me know what you think.

Thanks again,
Hal 

On Wed, 2012-01-11 at 22:16 -0600, Hal Finkel wrote:
> On Wed, 2012-01-11 at 18:45 -0800, Andrew Trick wrote:
> > On Dec 19, 2011, at 9:42 AM, Hal Finkel wrote:
> > 
> > > The current SelectionDAGBuilder does not allow loads to be reordered
> > > past stores, and does not allow stores to be reordered. This is a side
> > > effect of the way the critical chain is constructed: there is a queue of
> > > pending loads that is flushed (in parallel) to the root of the chain
> > > upon encountering any store (and that store is also appended to the root
> > > of the chain). Among other things, loop unrolling is far less effective
> > > than it otherwise could be.
> > > 
> > > The attached patch allows SelectionDAGBuilder to use the available alias
> > > analysis to reorder independent loads and stores. It changes the queue
> > > of pending loads into a more general queue of pending memory operations,
> > > and flushes, in parallel, all potentially-conflicting loads and stores
> > > as necessary.
> > 
> > Hal,
> > 
> > I haven't tried out your patch, so correct me if I'm wrong. I don't
> > understand how this can work with a simple alias analysis interface
> > like we have in LLVM.
> > 
> > Root
> > ld A
> > st B
> > st C
> > 
> > mayAlias(A,B) = true
> > mayAlias(A,C) = true
> > mayAlias(B,C) = false
> 
> This is a good point, your situation is something like this:
> 
> -------------------
> |        A        |
> |    B   |   C    |
> -------------------
> 
> and without strict aliasing rules, this could certainly be an issue.
> 
> > 
> > So we have the chain Root->A->B and Root->C which may result in the schedule:
> > 
> > st C
> > ld A
> > st B
> > 
> > It seems that we have to order all stores if we want them to "cover" preceding loads. What am I missing?
> > 
> 
> In the patch, aliasing loads and stores still set the current root to
> their combined token factor. As a result, the reordering will never be
> as severe as in your example; the patch is safe in this particular case
> because the DAG root will be set to A once B is visited and so C will
> also be forced to come after A. I'll need to give your point further
> thought, however, because maybe this kind of thing can bite in some
> other way.
> 
> We may need to build an aliasing set per chain in order to check for
> this kind of thing.
> 
> Thanks again,
> Hal
> 
> > -Andy
> 

-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm_lsro-20120116.diff
Type: text/x-patch
Size: 26136 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120116/cf799b00/attachment.bin>