This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Make DAGCombiner smarter about overflow
ClosedPublic

Authored by deadalnix on Feb 3 2017, 5:19 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

deadalnix created this revision.Feb 3 2017, 5:19 PM
RKSimon added inline comments.Feb 5 2017, 4:53 AM
include/llvm/CodeGen/SelectionDAG.h
1283 ↗(On Diff #87056)

Please document both the enum and the method.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1836 ↗(On Diff #87056)

SDLoc DL(N);

deadalnix updated this revision to Diff 87140.Feb 5 2017, 6:12 AM

Add documentation and style change.

RKSimon added inline comments.Feb 5 2017, 6:52 AM
include/llvm/CodeGen/SelectionDAG.h
1287 ↗(On Diff #87140)

result of

1290 ↗(On Diff #87140)

You can probably drop this part of the description now that OverflowKind is documented

1283 ↗(On Diff #87056)

http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Style guide says enums should ave a prefix: OFK_Never, OFK_Sometimes, OFK_Always

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2780 ↗(On Diff #87140)

Does the test cases cover both of these patterns? Its not clear if it does.

2782 ↗(On Diff #87140)

Are you intending to add OverflowKind::Always cases soon? If not you should possibly drop it from the enum and just put in a TODO.

deadalnix added inline comments.Feb 5 2017, 7:33 AM
lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2780 ↗(On Diff #87140)

I need some more improvement in the handling of adde/addc ( D29443 ) to expose the second one. then it kicks in in muladd in test/CodeGen/X86/adde-carry.ll

2782 ↗(On Diff #87140)

Yes I want to add some there to transform adde(X, C, Carry) => addc(X, C+1) when the carry is always there.

deadalnix updated this revision to Diff 87148.Feb 5 2017, 7:33 AM

Address comments.

RKSimon edited edge metadata.Feb 6 2017, 6:05 AM

One last comment.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2780 ↗(On Diff #87140)

In which case please can you drop this case until you can test for it? You can include it in another patch for review and commit it separately if you wish.

deadalnix updated this revision to Diff 87223.Feb 6 2017, 7:04 AM

Remove case that isn't testable at this time.

This revision was automatically updated to reflect the committed changes.

Crap I landed that one instead of D29521 . @RKSimon do you think it's good ? If not I'll revert.

Crap I landed that one instead of D29521 . @RKSimon do you think it's good ? If not I'll revert.

No you're ok - post LGTM.