[PATCH] D58911: DAG: Don't break value dependencies when sorting loads by offset

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:251
+      const SDNode *B = O2SMap[OffsetB];
+      if (A->isOperandOf(B))
+        return true;
----------------
There is unlikely but possible case neither is operand of another but depend through a third instruction.
Also what would happen if:

A: load i16 [base]
B: load i16 [base + 2]
C: load i8 [base + 1]

I guess normal sorting will tell A < C < B. With your change: B < A, A < C, C < B. That creates an impossible sort order.


================
Comment at: lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:270
+    // dependencies between some of the loads. Sort them when asking the target.
+    int64_t LowOffset = std::min(BaseOff, Offset);
+    int64_t HighOffset = std::max(BaseOff, Offset);
----------------
arsenm wrote:
> rampitec wrote:
> > Isn't that easier and cleaner to compute new BaseOff once yet while sorting?
> This is dependent on each offset so
> I don’t understand what you mean . The alternative is to delete the assert in the SI implementation, which isn’t really necessary
I mean you could compute min(Offsets[]) during sort and use it as a new base. Otherwise you may send different base offset to target with each call for the same chain.


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

https://reviews.llvm.org/D58911