Page MenuHomePhabricator

[DAGCombine] Deduplicate addcarry node using commutativity.
AcceptedPublic

Authored by deadalnix on Mon, Jan 28, 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.Mon, Jan 28, 2:34 AM
RKSimon added inline comments.Mon, Jan 28, 4:31 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2527

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

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

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.Mon, Jan 28, 6:56 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2527

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

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

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

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

yes

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

Add a comment about existing commutative ops deduplication logic.

This revision is now accepted and ready to land.Tue, Jan 29, 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 TranscriptThu, Feb 7, 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.