This is an archive of the discontinued LLVM Phabricator instance.

[X86] Don't pull carry through X86ISD::ADD carryin, -1 if we can't guranteed we're really using the carry flag from the add.
ClosedPublic

Authored by craig.topper on Aug 30 2017, 3:05 PM.

Details

Summary

Prior to this patch we had a DAG combine that tried to bypass an X86ISD::ADD with -1 being added to the carry flag of some previous operation. We would then pass the carry flag directly to user.

But this is only safe if the user is looking for the carry flag and not the zero flag.

So we need to only do this combine in a context where we know what flag the consumer is using.

Fixes PR34381.

Diff Detail

Event Timeline

craig.topper created this revision.Aug 30 2017, 3:05 PM
spatel edited edge metadata.Aug 31 2017, 11:28 AM

Some inline comments, but I haven't stepped through the actual problem yet...from the recent bug report comments, it seems more will be needed:
https://bugs.llvm.org/show_bug.cgi?id=34381

lib/Target/X86/X86ISelLowering.cpp
30972–30974

We have this explanatory comment above the helper, so no need to repeat it here and below.

34855–34856

Stray space diff?

35018–35019

Call this 'combineSBB' for symmetry with 'combineADC'. The x86-ness is implied by the mnemonics.

test/CodeGen/X86/pr34381.ll
12

Something's not right with this test - it passes on trunk already.

craig.topper added inline comments.Aug 31 2017, 11:44 AM
test/CodeGen/X86/pr34381.ll
12

it requires slowincdec. Which i though 64-bit defaulted to, but it turns out clang defaults the target-cpu to x86-64 in 64-bit mode with no other information and that enables slowincdec. llc defaults it to "generic". There were attributes in the original IR that I stripped out to reduce size.

hans added a subscriber: hans.Aug 31 2017, 11:51 AM

The X86ISelLowering.cpp change looks good to me.

I haven't looked closely at the test; if it already passes on trunk, that does seem like a problem :-)

Address Sanjay's comments. Add more command line options to the test case to get it to behave.

hans accepted this revision.Aug 31 2017, 12:46 PM
This revision is now accepted and ready to land.Aug 31 2017, 12:46 PM

@spatel does this look ok to you too?

spatel accepted this revision.Aug 31 2017, 2:24 PM

@spatel does this look ok to you too?

Sorry - didn't realize you were waiting on me. LGTM, although I still haven't stepped through the test case to know if it could be reduced.

This revision was automatically updated to reflect the committed changes.