This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Try harder to preserve NSW information for sext(sub) expressions
ClosedPublic

Authored by aemerson on Jul 11 2017, 7:00 AM.

Details

Summary

This helps to preserve NSW flags of sext(sub) expressions by trying to push the sign extension onto the operands of a sext instruction.

Diff Detail

Repository
rL LLVM

Event Timeline

aemerson created this revision.Jul 11 2017, 7:00 AM
fhahn added a subscriber: fhahn.Jul 11 2017, 7:57 AM
sanjoy requested changes to this revision.Jul 29 2017, 10:30 PM

Sorry for the delay, this one somehow slipped through the cracks.

lib/Analysis/ScalarEvolution.cpp
5966

Can you please use MatchBinaryOp here? That'll catch more cases than a literal sub instruction.

This revision now requires changes to proceed.Jul 29 2017, 10:30 PM
aemerson updated this revision to Diff 108895.Jul 31 2017, 4:05 AM
aemerson edited edge metadata.

Updated to use MatchBinaryOp.

sanjoy requested changes to this revision.Jul 31 2017, 10:09 AM
sanjoy added inline comments.
lib/Analysis/ScalarEvolution.cpp
5975

Why not use BO->IsNSW?

test/Analysis/ScalarEvolution/flags-from-poison.ll
612

Please add a test case that uses Intrinsic::ssub_with_overflow.

This revision now requires changes to proceed.Jul 31 2017, 10:09 AM
aemerson added inline comments.Aug 1 2017, 5:38 AM
test/Analysis/ScalarEvolution/flags-from-poison.ll
612

I see the code in MatchBinaryOperator doesn't seem to check for guard edges of the ssub_with_overflow intrinsic's overflow return value like it does for sadd_with_overflow. Is there a reason why it doesn't do that, if not I'll go and add support for it.

Without that modification it looks like intrinsic test case won't work.

sanjoy added inline comments.Aug 2 2017, 11:46 AM
test/Analysis/ScalarEvolution/flags-from-poison.ll
612

Is there a reason why it doesn't do that, if not I'll go and add support for it.

I don't think there was any reason beyond that that code would not be exercised (but this patch changes that); please go ahead with the implementation.

aemerson updated this revision to Diff 109731.Aug 4 2017, 7:24 AM
aemerson edited edge metadata.

Added support for handling the nsw/nuw ssub.with.overflow intrinsic.

sanjoy accepted this revision.Aug 4 2017, 10:10 AM

lgtm!

This revision is now accepted and ready to land.Aug 4 2017, 10:10 AM
This revision was automatically updated to reflect the committed changes.