[PATCH] D55073: [SelectionDAG] Split very large token factors for loads into 64k chunks

Fri Nov 30 13:22:47 PST 2018

aemerson marked an inline comment as done and an inline comment as not done.
aemerson added a comment.

In D55073#1313703 <https://reviews.llvm.org/D55073#1313703>, @RKSimon wrote:

> Does this fix PR7250 and PR37000 ? Those test cases look usable.

PR7250 asserts in a different place, for the number of result values of a node. This issue is very similar, but instead exceeds the number of operands.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1052
+    Root =
+        DAG.getNode(ISD::TokenFactor, getCurSDLoc(), MVT::Other, TokenFactors);
+  }
----------------
efriedma wrote:
> aemerson wrote:
> > efriedma wrote:
> > > Instead of making a tree of TokenFactors, could you make a list?  It seems a little simpler (less code, and you don't have to worry about the length of TokenFactors itself).
> > > 
> > > I'm a little worried that other code dealing with TokenFactors might end up violating the limit if we're very close... any idea if there's other code that could be affected, like DAGCombine?  Do we have an assertion somewhere that will reliably catch this issue?
> > Could you clarify what you mean by list?
> I mean, each TokenFactor should only have one TokenFactor operand.  So the algorithm would be something like this: remove "Limit" loads from end of PendingLoads, make a TokenFactor, and append the resulting TokenFactor to PendingLoads.  Repeat as necessary.
Ok, I could do that. As for the other places, since SDAG will CSE nodes we should be able to catch this case in getNode() somewhere and assert.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55073