[PATCH] D56291: ScheduleDAG: Don't break the dependence in clustering neighboring loads.

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:258
+    // We continue to search for other candidates.
+    if (Load->isOperandOf(BaseLoad))
+      continue;
----------------
cfang wrote:
> rampitec wrote:
> > I see two flaws in this logic:
> > 
> > 1. A load can depend on any previous load, not just base.
> > 2. A dependency can be indirect, not necessarily as a direct operand.
> > 
> > The problem is if you use a loop over the whole chain and also use isPredecessorOf instead of isOperandOf that will make this loop extremely expensive. It needs to be somehow simplified. Maybe do such check only if getIROrder() of successor is less or equal getIROrder() of a predecessor in the assumption if it was located higher in the IR and given all of that happens in the same basic block, there can be no dependency?
> >1.  A load can depend on any previous load, not just base.
> Right. But here we should only care about whether the baseload depends on this load because we are trying to create a dependence of the load on the baseload by glueing.
> 
> >2. A dependency can be indirect, not necessarily as a direct operand.
> llvm::sort(Offsets);
> All the leading loads have been checked already, so I am assuming no indirect dependence.  
You are only creating dependency on a base load simply because that is the testcase you are using. All loads in chain are glued together. You can make a test: start load chain with an independent load and then have these two. You will run into the same issue.

All the loads were checked, so you should have assumed no direct dependency as well. Although both generic code and target callback missed that dependency anyway. I do not see why it would detect indirect dependency if it does not handle direct.

Thinking about this I am not sure this is a right place to handle it. To cluster loads they should be from the same base and also all linked the the same base chain. This should result in a no dependency. That is the reason why sorting by offset is legal, there is no dependency between subsequent loads.

Now I have a question: why two of our loads in question do not model dependency at the DAG level? This seems to be a real problem to me which needs to be fixed. I assume two subreg loads in question shall be chained together in a correct order to prevent rescheduling.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56291/new/

https://reviews.llvm.org/D56291