Page MenuHomePhabricator

[SCEV] Fix runtime error caused by ValueOffsetPair
ClosedPublic

Authored by wmi on Jul 28 2016, 2:51 PM.

Details

Summary

The patch is to fix the bug in https://llvm.org/bugs/show_bug.cgi?id=28705

Previously in SCEVExpander::findExistingExpansion, we returned the value part of the ValueOffsetPair returned by FindValueInExprValueMap directly no matter whether the offset is zero or not. I mistakenly thought SCEVExpander::findExistingExpansion was only used to check whether there was an existing value to be reused (This is true for all its uses in lib/Analysis/ScalarEvolutionExpander.cpp). I missed the fact that SCEVExpander::findExistingExpansion was also used in IndVarSimplify::expandSCEVIfNeeded and the value returned was used to replace exit value of induction variable in IndVarSimplify. It caused IndVarSimplify used a wrong value as the exit value of induction variable (The correct value should be "value - offset" expr).

The fix is to add an OnlyCheckIsNull param for SCEVExpander::findExistingExpansion. If OnlyCheckIsNull is false and ValueOffsetPair returned by FindValueInExprValueMap contains a non-zero offset, we will return nullptr. So that IndVarSimplify::expandSCEVIfNeeded will continue to use SCEVExpander::expandCodeFor to do the expansion.

I don't want SCEVExpander::findExistingExpansion to insert the "Value - Offset" expr based on ValueOffsetPair because it can insert the expr inside the loop. SCEVExpander::expandCodeFor contains the logic to choose a better insert point.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi updated this revision to Diff 66000.Jul 28 2016, 2:51 PM
wmi retitled this revision from to [SCEV] Fix runtime error caused by ValueOffsetPair.
wmi updated this object.
wmi added a reviewer: sanjoy.
wmi set the repository for this revision to rL LLVM.
wmi added subscribers: llvm-commits, davidxl, hans.
sanjoy requested changes to this revision.Jul 28 2016, 6:35 PM
sanjoy edited edge metadata.
sanjoy added inline comments.
include/llvm/Analysis/ScalarEvolutionExpander.h
283 ↗(On Diff #66000)

Hi Wei,

I'm not sure this is the best interface -- it still is too easy to make mistakes. I think there should be two entry points here (which can share code as appropriate):

  • Value *getExactExistingExpansion : if this returns non-null, then we know that the return value is the exact expansion. We'll use this in IndVarSimplify (and potentially other places that don't want to generate additional code).
  • Optional<ValueOffsetPair> getRelatedExistingExpansion: if this returns non- None, then we know we can codegen the ValueOffsetPair into a suitable expansion.

The first can be implemented in terms of the latter.

This revision now requires changes to proceed.Jul 28 2016, 6:35 PM
wmi added inline comments.Jul 28 2016, 11:33 PM
include/llvm/Analysis/ScalarEvolutionExpander.h
283 ↗(On Diff #66000)

That is a good suggestion. Thanks.

wmi updated this revision to Diff 66081.Jul 28 2016, 11:34 PM
wmi edited edge metadata.

Address Sanjoy's comments.

sanjoy requested changes to this revision.Aug 5 2016, 6:03 PM
sanjoy edited edge metadata.

Just to be clear, you're going to squash this with rL276136 and check in the result?

include/llvm/Analysis/ScalarEvolutionExpander.h
274 ↗(On Diff #66081)

s/func/function

Also, I'm not sure if we need to mention getRelatedExistingExpansion etc. here. I'd just directly state that it gives an expansion for S and nothing more.

282 ↗(On Diff #66081)

s/find/find the/ or s/find/find a/
s/func/function/

282 ↗(On Diff #66081)

We don't use \brief in new code.

284 ↗(On Diff #66081)

s/If only return is non-None/If this returns a non-None value/

lib/Analysis/ScalarEvolutionExpander.cpp
1998 ↗(On Diff #66081)

I don't think you need the hasValue. Applies to all the places where you've used the Optional as a boolean.

test/Analysis/ScalarEvolution/pr28705.ll
2 ↗(On Diff #66081)

Why do you need debug-only?

This revision now requires changes to proceed.Aug 5 2016, 6:03 PM
wmi added a comment.Aug 5 2016, 10:12 PM

Just to be clear, you're going to squash this with rL276136 and check in the result.

I can checkin them consecutively. which way do you think is better?

include/llvm/Analysis/ScalarEvolutionExpander.h
274 ↗(On Diff #66081)

I wanted to emphasize their difference since they have similar names but different usages.

I simplified the comment to: /// Try to find existing LLVM IR value for S available at the point At.

282 ↗(On Diff #66081)

Fixed.

284 ↗(On Diff #66081)

Fixed.

lib/Analysis/ScalarEvolutionExpander.cpp
1998 ↗(On Diff #66081)

Oh, that looks better. Thanks.

test/Analysis/ScalarEvolution/pr28705.ll
2 ↗(On Diff #66081)

That is a careless mistake. Removed.

wmi updated this revision to Diff 67064.Aug 5 2016, 10:15 PM
wmi edited edge metadata.

Address Sanjoy's comment.

sanjoy accepted this revision.Aug 9 2016, 11:15 AM
sanjoy edited edge metadata.

lgtm

test/Analysis/ScalarEvolution/pr28705.ll
13 ↗(On Diff #67064)

This is totally optional, but if I'd clean up the variable names a little bit before checkin. E.g. s/%I641.03128/%iv, s/ %DB.sroa.9.03127/%iv2 etc.

This revision is now accepted and ready to land.Aug 9 2016, 11:15 AM
wmi added inline comments.Aug 9 2016, 12:33 PM
test/Analysis/ScalarEvolution/pr28705.ll
13 ↗(On Diff #67064)

Fixed. Thanks.

wmi updated this revision to Diff 67394.Aug 9 2016, 12:34 PM
wmi edited edge metadata.

Fix variable names in test.

This revision was automatically updated to reflect the committed changes.