This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Add getTokenFactor, which splits nodes with > 64k operands.
ClosedPublic

Authored by fhahn on Jan 15 2019, 12:00 PM.

Details

Summary

This functionality is required at multiple places which potentially
create large operand lists, like SelectionDAGBuilder or DAGCombiner.

Also updates the limit computation to use SDNode::getMaxNumOperands.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Jan 15 2019, 12:00 PM
efriedma added inline comments.Jan 15 2019, 12:25 PM
llvm/include/llvm/CodeGen/SelectionDAG.h
1161 ↗(On Diff #181846)

This API seems strange... could we just add a "getTokenFactor" API instead?

@fhahn Are you intending to look at the similar SDNode::NumValues limit? PR7250 + PR37000 ?

fhahn updated this revision to Diff 182144.Jan 16 2019, 1:59 PM
fhahn retitled this revision from [SelectionDAG] Add splitAcrossTokenFactors to split nodes with > 64k operands. to [SelectionDAG] Add getTokenFactor, which splits nodes with > 64k operands..

Thanks Eli, changed to getTokenFactor.

fhahn added a comment.Jan 16 2019, 2:01 PM

@fhahn Are you intending to look at the similar SDNode::NumValues limit? PR7250 + PR37000 ?

I am primarily trying to fix SelectionDAG to deal with programs with a very large number of stores at the moment. But I could have a look at the mentioned issues in a bit and see if the same solution is also applicable.

RKSimon added inline comments.Jan 16 2019, 2:06 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
9291 ↗(On Diff #182144)

Maybe better to use an ArrayRef<SDValue> here instead of modifying an external value?

9292 ↗(On Diff #182144)

Would it be better to create a SDNode::getMaxNumOperands() static/constexpr helper to do this?

@fhahn Are you intending to look at the similar SDNode::NumValues limit? PR7250 + PR37000 ?

I am primarily trying to fix SelectionDAG to deal with programs with a very large number of stores at the moment. But I could have a look at the mentioned issues in a bit and see if the same solution is also applicable.

No problem, just trying to ensure those bugs stay on the radar. Thanks.

fhahn updated this revision to Diff 182303.Jan 17 2019, 8:46 AM
fhahn edited the summary of this revision. (Show Details)

updated to use SDNode::getMaxNumOperands

fhahn marked 2 inline comments as done.Jan 17 2019, 8:47 AM
fhahn added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
9291 ↗(On Diff #182144)

The reason it modifies an external value is that it allows us to nicely collapse the incoming values into TokenFactors, without needing to allocate an additional array. I changed it to take a SmallVectorImpl, to make it more generic. Does that make sense?

RKSimon accepted this revision.Jan 17 2019, 1:18 PM

LGTM

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
9291 ↗(On Diff #182144)

I understand, I'm still a little worried but given how infrequent this is likely to be called it should be OK.

This revision is now accepted and ready to land.Jan 17 2019, 1:18 PM
This revision was automatically updated to reflect the committed changes.