This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Push truncate through adde when the carry isn't used.
ClosedPublic

Authored by deadalnix on Feb 3 2017, 6:26 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

deadalnix created this revision.Feb 3 2017, 6:26 PM
arsenm added a subscriber: arsenm.Feb 3 2017, 6:32 PM
arsenm added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
7781 ↗(On Diff #87064)

Extra whitespace change

7870–7872 ↗(On Diff #87064)

Needs a legality check for the resulting adde?

deadalnix updated this revision to Diff 87092.Feb 4 2017, 5:31 AM

Check for adde's validity before combining.

arsenm added inline comments.Feb 4 2017, 10:21 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
7874 ↗(On Diff #87092)

Usually this is done as !LegalizeDAG || TLI.isOperationLegal..

7875 ↗(On Diff #87092)

SDLoc SL(N)

test/CodeGen/X86/adde-carry.ll
109–110 ↗(On Diff #87092)

Can you add tests with more types?

deadalnix marked an inline comment as done.Feb 4 2017, 1:24 PM
deadalnix added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
7874 ↗(On Diff #87092)

There is no wrapper for this ?

deadalnix updated this revision to Diff 87119.Feb 4 2017, 3:22 PM

Add test cases for various sizes.
Use SDLoc SL(N) .

RKSimon added inline comments.Feb 6 2017, 6:10 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
7872 ↗(On Diff #87119)

clang-format

deadalnix updated this revision to Diff 87232.Feb 6 2017, 7:53 AM

Rebase, clang-format

RKSimon added inline comments.Feb 6 2017, 10:55 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
7876 ↗(On Diff #87232)

This should probably be (!LegalOperations || TLI.isOperationLegal(ISD::ADDE, VT)) - otherwise you might get into trouble if custom lowering results in the truncate being regenerated.

7879 ↗(On Diff #87232)

Typically we don't use auto for these cases - just use SDvalue

deadalnix updated this revision to Diff 87289.Feb 6 2017, 1:11 PM

Use (!LegalOperations || TLI.isOperationLegal(ISD::ADDE, VT))

arsenm accepted this revision.Feb 7 2017, 3:18 PM

LGTM

This revision is now accepted and ready to land.Feb 7 2017, 3:18 PM
This revision was automatically updated to reflect the committed changes.