This is an archive of the discontinued LLVM Phabricator instance.

[Reassociate] Better preserve NSW/NUW flags. (PR12985)
ClosedPublic

Authored by mcrosier on Nov 7 2014, 10:28 AM.

Details

Summary

This change better preserves the NSW/NUW flags in the reassociation pass.

Part of PR12985.

Diff Detail

Event Timeline

mcrosier updated this revision to Diff 15932.Nov 7 2014, 10:28 AM
mcrosier retitled this revision from to [Reassociate] Better preserve NSW/NUW flags. (PR12985).
mcrosier updated this object.
mcrosier edited the test plan for this revision. (Show Details)
mcrosier added reviewers: atrick, majnemer, chandlerc.
mcrosier added a subscriber: Unknown Object (MLST).Nov 7 2014, 10:56 AM
majnemer edited edge metadata.Nov 7 2014, 11:44 AM

This transform is not safe for nsw shl, it will transform %shl = shl nsw i32 %a, 31 into %shl = mul nsw i32 %a, -2147483648

If %a is -1, %shl should hold INT_MIN. Instead, it will transform it into poison.

However, I think it's fine to turn a nuw shl into an nuw mul in all cases.
I think you can also turn an nuw nsw shl into an nuw nsw mul.

test/Transforms/Reassociate/wrap-flags.ll
16–27

This transform isn't sound if %y is INT_MIN, %add would be transformed from something valid into poison.

mcrosier updated this revision to Diff 15936.Nov 7 2014, 12:24 PM
mcrosier edited edge metadata.

Addressed David's comments.

majnemer added inline comments.Nov 7 2014, 12:33 PM
lib/Transforms/Scalar/Reassociate.cpp
1050–1056

Shouldn't this be:

if (NSW && NUW)

?

mcrosier added inline comments.Nov 7 2014, 12:56 PM
lib/Transforms/Scalar/Reassociate.cpp
1050–1056

The logic is the same, but in the case that NSW is false it will avoid the call to the ineffectual setHasNoSignedWrap.

I'll fix this.. thanks!

mcrosier updated this revision to Diff 15937.Nov 7 2014, 12:59 PM

Minor tweak to avoid unnecessary call to setter.

majnemer accepted this revision.Nov 7 2014, 2:15 PM
majnemer edited edge metadata.

LGTM with tweaks.

lib/Transforms/Scalar/Reassociate.cpp
1056

This can just be Mul->setHasNoSignedWrap(true);

This revision is now accepted and ready to land.Nov 7 2014, 2:15 PM
mcrosier closed this revision.Nov 7 2014, 2:24 PM

Thanks, David! Committed in r221555.