[LLVMdev] DAGCombiner::MergeConsecutiveStores

[Mehdi Amini]

> On Feb 13, 2015, at 5:50 AM, Mikael Holmén <mikael.holmen at ericsson.com> wrote:
> 
> Hi,
> 
> I'm quite puzzled by a little bit of code in the DAGCombiner where it merges loads in MergeConsecutiveStores.
> 
> Two 16bit loads have been merged to one 32bit load, and two 16bit stores have been combined to one 32bit store.
> 
> And then the code goes like this:
> 
> 
>  // Replace one of the loads with the new load.
>  LoadSDNode *Ld = cast<LoadSDNode>(LoadNodes[0].MemNode);
>  DAG.ReplaceAllUsesOfValueWith(SDValue(Ld, 1),
>                                SDValue(NewLoad.getNode(), 1));


At this point the Chain that the next Load gets has been updated to the new chain, ok?

> 
>  // Remove the rest of the load chains.
>  for (unsigned i = 1; i < NumElem ; ++i) {
>    // Replace all chain users of the old load nodes with the chain of the new
>    // load node.
>    LoadSDNode *Ld = cast<LoadSDNode>(LoadNodes[i].MemNode);
>    DAG.ReplaceAllUsesOfValueWith(SDValue(Ld, 1), Ld->getChain());

Ld->getChain() return the Chain that is passed as an input to the Load, it has been replaced earlier.

Let say your original loads are chained this way A->B->C. The new load is A’. Before the loop, A has been replaced with A’:

A’->B->C

Then the first iteration of the loop process B and replace its users with the chain it has as an input:

A’->C

And so on.

I hope it makes sense.


Mehdi



>  }
> 
> And here I can't understand why we should replace "the rest" of the load chains with the loads' getChain(). Why should one load be treated in one way, and the rest in some other way?
> 
> I.e. if there is a chain dependendy to a load, we replace that with the load's chain. Why not replace dependencies to the old loads with dependencies to the new load, just like we do for the first load in the code above.
> 
> This code was rewritten to how it looks today in a commit 2012-10-03:
> 
> "    Fix a cycle in the DAG. In this code we replace multiple loads
>     with a single load and
>     multiple stores with a single load. We create the wide loads and
>     stores (and their chains)
>     before we remove the scalar loads and stores and fix the DAG chain.
>     We attempted to merge loads with a different chain. When that
>     happened, the assumption that it is safe to RAUW
>     broke and a cycle was introduced.
> 
>    git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@165148 91177308-0d34-0410-b5e6-96231b3b80d8
> "
> 
> The testcase in that commit works even if I treat all loads the same and replace them all with SDValue(NewLoad.getNode(), 1)
> 
> Anyone knows this code and why it looks the way it does?
> 
> Regards,
> Mikael
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev