This is an archive of the discontinued LLVM Phabricator instance.

[X86] Extend load-op-store fusion merge to ADC/SBB.
ClosedPublic

Authored by niravd on Jan 16 2018, 1:05 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

niravd created this revision.Jan 16 2018, 1:05 PM

Missing tests for all of SBB and most ADC types.

craig.topper added inline comments.Jan 16 2018, 3:01 PM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
2322 ↗(On Diff #130018)

This part also needs a test case though I'm not even sure its mathematically sound.

For an 8-bit add for example, it turns (X + 128) into (X - (-128)) because since we can fit -128 into a sign extended 8-bit immediate, but not 128.

For an ADC, I think this code would try to turn (X + 128 + C) into (X - (-128) - C) since SBB subtracts the carry. So I don't think that works.

niravd updated this revision to Diff 130246.Jan 17 2018, 12:07 PM

Add ADC immediate tests. I haven't worked out a way to the appropriate SUB immediate nodes via LLVM IR as they're realized via add.

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
2322 ↗(On Diff #130018)

Agreed. Reverted.

craig.topper added inline comments.Jan 17 2018, 4:33 PM
llvm/test/CodeGen/X86/addcarry2.ll
234 ↗(On Diff #130246)

The test name and this check line don't match

252 ↗(On Diff #130246)

Same here

Unless I'm crazy the entirety of addcarry2.ll already passes on trunk. I think we hit regular isel patterns unless the carry out of the ADC/SBB is used.

Unless I'm crazy the entirety of addcarry2.ll already passes on trunk. I think we hit regular isel patterns unless the carry out of the ADC/SBB is used.

You are correct these case are already handled, but they're now being matched here so this at least shows we're mimicking the default instruction selection .

I've found a valid test case for generating ADCs of with the same length as the address size but not the smaller ones. I'll update with those test for 32 and 64. No luck with SBB immediate tests though. I'll update the patch presently.

niravd updated this revision to Diff 130463.Jan 18 2018, 11:26 AM

update test

niravd marked 2 inline comments as done.Jan 18 2018, 11:26 AM
This revision is now accepted and ready to land.Jan 18 2018, 4:54 PM
This revision was automatically updated to reflect the committed changes.