Page MenuHomePhabricator

[DAGCombine] Deduplicate addcarry node using commutativity.
AcceptedPublic

Authored by deadalnix on Jan 28 2019, 2:34 AM.

Details

Summary

The first two parameters of addcarry are commutative. We may face a situation where both variant are present in the DAG, in which case we benefit from using just one.

Depends on D57302 and D33587

Diff Detail

Event Timeline

deadalnix created this revision.Jan 28 2019, 2:34 AM
RKSimon added inline comments.Jan 28 2019, 4:31 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2900

We already do this in DAGCombiner::combine for plain commutative binops, is it worth generalizing that code?

deadalnix marked an inline comment as done.Jan 28 2019, 6:10 AM
deadalnix added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2900

The problem is that this is not a binary operation so it doesn't fit in the existing machinery. If this end up being common enough, we can generalize.

RKSimon added inline comments.Jan 28 2019, 6:56 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2900

OK - please add a TODO suggesting this. Should the ConstantSDNode handling be done here as well?

deadalnix marked an inline comment as done.Jan 28 2019, 10:30 AM
deadalnix added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2900

You mean moving a constant argument on RHS when it is on RHS ?

RKSimon added inline comments.Jan 28 2019, 10:37 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2900

yes

deadalnix updated this revision to Diff 183965.Jan 28 2019, 2:52 PM

Add a comment about existing commutative ops deduplication logic.

This revision is now accepted and ready to land.Jan 29 2019, 12:51 AM

@deadalnix Can this go in now or would you prefer to wait for D33587?

Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 4:44 AM

@RKSimon This can go now, but has no effect in isolation in any of the test cases I have, mainly because DAGCombine is fairly powerless when faced with addcarry as most of the work involve 'deep' patterns. Do you think I should land anyways ?

@RKSimon This can go now, but has no effect in isolation in any of the test cases I have, mainly because DAGCombine is fairly powerless when faced with addcarry as most of the work involve 'deep' patterns. Do you think I should land anyways ?

Unless you can add test coverage now I think this will have to wait.

@deadalnix What is the state of this patch now?

deadalnix updated this revision to Diff 207928.Jul 3 2019, 5:00 PM

Rebased on top of D64174 , which expose many instances of this pattern.