Page MenuHomePhabricator

[DAGCombiner] Use getTokenFactor in a few more cases.
ClosedPublic

Authored by fhahn on Mar 8 2019, 2:04 PM.

Details

Summary

SDNodes can only have 64k operands and for some inputs (e.g. large
number of stores), we can reach this limit when creating TokenFactor
nodes. This patch is a follow up to D56740 and updates a few more places
that potentially can create TokenFactors with too many operands.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Mar 8 2019, 2:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2019, 2:04 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Any chance of test cases?

fhahn added a comment.Mar 14 2019, 8:16 AM

Unfortunately this requires at least 64k stores to trigger AFAIK. It is triggered by the same input as D56740. I could try to reduce it, but I am not sure if such a big test case would be valuable?

RKSimon accepted this revision.Mar 14 2019, 9:49 AM

TBH I didn't expect that tests would be easy, but I don't think that should be a stopper to this patch (and code coverage indicates that all these cases are active).

LGTM

This revision is now accepted and ready to land.Mar 14 2019, 9:49 AM
fhahn added a comment.Mar 15 2019, 5:53 AM

TBH I didn't expect that tests would be easy, but I don't think that should be a stopper to this patch (and code coverage indicates that all these cases are active).

Thanks! I'll try to reduce it over the weekend. Also, this might be something that would be good to fuzz.

TBH I didn't expect that tests would be easy, but I don't think that should be a stopper to this patch (and code coverage indicates that all these cases are active).

I've got a simple python script to generate a test case, but it takes multiple minutes, because DAGCombine is really slow with lots of stores (which is another thing I am looking into). For now, I think the patch has to go in without test case :(

This revision was automatically updated to reflect the committed changes.