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.

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

Please document both the enum and the method.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1850

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
1283

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

1287

result of

1290

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

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2780

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

2782

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

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

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

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.