This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] choose 1 form of abs and nabs as canonical
ClosedPublic

Authored by spatel on May 18 2018, 9:24 AM.

Details

Summary

We already do this for min/max (see the blob above the diff), so we should do the same for abs/nabs.
Compare with 0 and having the original argument as the true value of the select feels the most natural to me, but if someone prefers a different flavor, we could do that too.

This might solve the motivating cases for D47037 and D47041, but I think those patches still make sense. We can't guarantee this canonicalization if the icmp has more than one use.

I adjusted the shifty abs transform to the pattern here to make that transform more efficient.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.May 18 2018, 9:24 AM
spatel updated this revision to Diff 147535.May 18 2018, 9:41 AM

Patch updated:
No code changes, but I forgot to update test comments that would become stale.

I guess it doesn't really matter what form you choose as canonical, given the backend will pattern-match it anyway, but "x > 0" seems like a weird choice; it can't be lowered to a sign-bit check like "x < 0".

spatel added a comment.EditedMay 18 2018, 1:04 PM

I guess it doesn't really matter what form you choose as canonical, given the backend will pattern-match it anyway, but "x > 0" seems like a weird choice; it can't be lowered to a sign-bit check like "x < 0".

Good point. It should make the code simpler if we always choose <0 too. Let me update.

spatel updated this revision to Diff 147578.May 18 2018, 1:45 PM

Patch updated:
Always choose "<s 0" because a sign-bit check and zero constant are most likely to match what codegen prefers.

This revision is now accepted and ready to land.May 18 2018, 1:51 PM
This revision was automatically updated to reflect the committed changes.