This is an archive of the discontinued LLVM Phabricator instance.

Reapply "InstCombine: Reduce trunc (shl x, K) width."
ClosedPublic

Authored by arsenm on Jul 11 2016, 1:26 PM.

Details

Reviewers
majnemer
Summary

This reapplies r272987 with a fix for infinitely looping
when the truncated value is another shift of a constant.

Diff Detail

Event Timeline

arsenm updated this revision to Diff 63562.Jul 11 2016, 1:26 PM
arsenm retitled this revision from to Reapply "InstCombine: Reduce trunc (shl x, K) width.".
arsenm updated this object.
arsenm added a reviewer: majnemer.
arsenm added a subscriber: llvm-commits.
majnemer edited edge metadata.Jul 27 2016, 8:00 PM

Sorry for the delay, I forgot to hit submit...

lib/Transforms/InstCombine/InstCombineCasts.cpp
547

isn't non-native

Can we simplify that down to "native" ?

test/Transforms/InstCombine/cast.ll
990 ↗(On Diff #63562)

This seems like a pessimization. Any ideas?

arsenm updated this revision to Diff 65962.Jul 28 2016, 11:31 AM
arsenm edited edge metadata.
arsenm marked 2 inline comments as done.

Fix unnecessary test change, comment

test/Transforms/InstCombine/cast.ll
990 ↗(On Diff #63562)

This is leftover junk, the test change isn't necessary

majnemer added inline comments.Jul 28 2016, 5:48 PM
lib/Transforms/InstCombine/InstCombineCasts.cpp
534–535

This comment seems duplicated.

547

Can we replace "isn't non-native" with "is native"?

548

Consider using m_APInt here, it handles vector splats.

552

I think it'd be nice to use getScalarSizeInBits so that the code is more vector ready.

test/Transforms/InstCombine/2011-05-28-swapmulsub.ll
36

What happens with this i16?

arsenm updated this revision to Diff 71211.Sep 13 2016, 11:30 AM

Add vector tests, check i16 use

majnemer accepted this revision.Sep 13 2016, 12:13 PM
majnemer edited edge metadata.
majnemer added inline comments.
lib/Transforms/InstCombine/InstCombineCasts.cpp
540

Can m_ConstantInt be m_Constant here?

548

Could you use m_APInt instead of m_ConstantInt?

This revision is now accepted and ready to land.Sep 13 2016, 12:13 PM
arsenm added inline comments.Sep 13 2016, 12:22 PM
lib/Transforms/InstCombine/InstCombineCasts.cpp
548

This just adds more clutter without changing the other code before this since it's re-using Cst and the whole block is already skipped for !isa<IntegerType>

arsenm added inline comments.Sep 13 2016, 12:25 PM
lib/Transforms/InstCombine/InstCombineCasts.cpp
540

This would also require an additional variable since Cst is ConstantInt

majnemer added inline comments.Sep 13 2016, 12:27 PM
lib/Transforms/InstCombine/InstCombineCasts.cpp
540

I think we can afford an extra variable.

548

OK.

arsenm closed this revision.Sep 13 2016, 12:52 PM

r281379