Page MenuHomePhabricator

DAG: avoid truncating a sign extended operand when test equality against zero

Authored by weimingz on Jul 28 2016, 1:19 PM.



When an operand is tested for EQ/NE against zero and the operand is sign extended, the truncate instruction is not needed.

E.g. given

if (short_val == 0)

It currently generates code for ARM like

movw r1, #65535
tst r0, r1
it eq

instead of
cbz r0

Diff Detail


Event Timeline

weimingz updated this revision to Diff 65976.Jul 28 2016, 1:19 PM
weimingz retitled this revision from to DAG: avoid truncating a sign extended operand when test equality against zero .
weimingz updated this object.
weimingz set the repository for this revision to rL LLVM.
weimingz added a subscriber: llvm-commits.
t.p.northover added inline comments.
959–960 ↗(On Diff #65976)

The comment should probably be in the block it belongs to.

961 ↗(On Diff #65976)

I think there are a couple of issues here.

First, I think you need to check that the AssertSext is from the same type (or smaller) as the original values (like above). Otherwise there could still be unknown rubbish in between the two cut-offs.

Second, don't all of these cases apply equally to AssertZext?

52–54 ↗(On Diff #65976)

This looks unrelated?

weimingz marked an inline comment as done.Jul 28 2016, 4:44 PM
weimingz added inline comments.
961 ↗(On Diff #65976)

AssertZExt is handled in later DAG combining because it can computes the known bits.

52–54 ↗(On Diff #65976)

sorry. I added the wrong file. I suppose to add the test case for it.

weimingz updated this revision to Diff 66057.Jul 28 2016, 4:51 PM
weimingz updated this object.
eli.friedman added inline comments.
964 ↗(On Diff #66057)

Could you just use OpL->ComputeNumSignBits() > ExtensionBits && OpR->ComputeNumSignBits() > ExtensionBits or something like that here?

weimingz updated this revision to Diff 66068.Jul 28 2016, 6:06 PM

@Eli please review the new patch.

I think you missed the point of my suggestion... as long as you can prove both sides have enough sign bits, it doesn't matter what kind of node you're dealing with.

weimingz updated this revision to Diff 66080.Jul 29 2016, 12:11 AM
weimingz updated this revision to Diff 66087.Jul 29 2016, 12:17 AM

@Eli, is patch set 5 what you mean? I'm more confident with patch set 4 though. :)

It would be nice to have a couple testcases involving numbers which aren't zero.

956 ↗(On Diff #66087)

This check is now unnecessary.

964 ↗(On Diff #66087)

Maybe reorganize this a bit to make it more readable? A couple variables might be helpful.

eli.friedman edited edge metadata.

Oh, and yes, this version is what I was thinking of.

weimingz added inline comments.Jul 29 2016, 11:44 AM
956 ↗(On Diff #66087)

Yes. I was thinking the same. But my concern is computeNumSignBits only gives its best knowledge by looking into no more than 6 steps. Will it miss some cases?

eli.friedman added inline comments.Jul 29 2016, 12:11 PM
956 ↗(On Diff #66087)

It definitely won't miss this case, which doesn't require any recursion at all. When computeNumSignBits hits the recursion limit, it doesn't completely abandon the computation; it just treats the SDValue at the recursion limit as an opaque value.

weimingz updated this revision to Diff 66182.Jul 29 2016, 3:03 PM
weimingz edited edge metadata.

Address Eli's comments

eli.friedman accepted this revision.Jul 29 2016, 3:15 PM
eli.friedman edited edge metadata.
eli.friedman added inline comments.
10 ↗(On Diff #66182)

Some of these tests look a little fragile; maybe generate checks with

Otherwise LGTM.

This revision is now accepted and ready to land.Jul 29 2016, 3:15 PM
weimingz updated this revision to Diff 66191.Jul 29 2016, 3:48 PM
weimingz edited edge metadata.

Thanks Eli for reviewing.
Is the tool for x86 only?

Anyway, I added some reg exp to let the CHECK rule be more generic

This revision was automatically updated to reflect the committed changes.

Oh, I wasn't really paying attention to that... I guess it is? If it doesn't work, that's fine. :) The tests look better now.