This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Be careful with nuw/nsw/exact in InsertBinop
ClosedPublic

Authored by skatkov on Dec 26 2017, 1:23 AM.

Details

Summary

InsertBinop tries to find an appropriate instruction instead of
creating a new instruction. When it checks whether instruction is
the same as we need to create it ignores nuw/nsw/exact flags.

It leads to invalid behavior when poison instruction can be used
when it was not expected. Specifically, for example Expander
expands the SCEV built for instruction
%a = add i32 %v, 1
It is possible that InsertBinop can find an instruction
% b = add nuw nsw i32 %v, 1
and will use it instead of version w/o nuw nsw.
It is incorrect.

The patch conservatively ignores all instructions with any of
poison flags installed.

Diff Detail

Repository
rL LLVM

Event Timeline

skatkov created this revision.Dec 26 2017, 1:23 AM
sanjoy requested changes to this revision.Dec 26 2017, 1:23 PM
sanjoy added inline comments.
lib/Analysis/ScalarEvolutionExpander.cpp
198 ↗(On Diff #128153)

Good catch!

I think you also need to check for I having the exact bit set since it looks like InsertBinop is used for shifts and divisions as well.

Finally, please also add a TODO that we can be more aggressive here by piping in the flags for the SCEV expression whose expansion we want to insert (nsw/nuw/exact).

This revision now requires changes to proceed.Dec 26 2017, 1:23 PM
skatkov updated this revision to Diff 128194.Dec 26 2017, 9:29 PM
skatkov retitled this revision from [SCEV] Be careful with nuw/nsw in InsertBinop to [SCEV] Be careful with nuw/nsw/exact in InsertBinop.
skatkov edited the summary of this revision. (Show Details)
sanjoy accepted this revision.Dec 26 2017, 10:03 PM

lgtm!

lib/Analysis/ScalarEvolutionExpander.cpp
204 ↗(On Diff #128194)

I also think s/hasPoisonFlags`/canGeneratePoison/ sounds better.

This revision is now accepted and ready to land.Dec 26 2017, 10:03 PM
This revision was automatically updated to reflect the committed changes.