This is an archive of the discontinued LLVM Phabricator instance.

[LVI] Make use of 'assume'-provided data
AbandonedPublic

Authored by junparser on Jun 10 2020, 2:20 AM.

Details

Summary

This patch fix regression showed in https://bugs.llvm.org/show_bug.cgi?id=45686.
It improved LVI in two ways: 1) analysis value in entry block. 2) combine assumptions with value range when solve selectInst in solveBlockValueSelect

TestPlan: check-llvm

Diff Detail

Event Timeline

junparser created this revision.Jun 10 2020, 2:20 AM
lebedev.ri retitled this revision from Fix pr45686 to [LVI] Make use of 'assume'-provided data.Jun 10 2020, 2:44 AM
fhahn added a comment.Jun 10 2020, 2:59 AM

It improved LVI in two ways: 1) analysis value in entry block. 2) combine assumptions with value range when solve selectInst in solveBlockValueSelect

It would probably be best to submit this as 2 separate patches.

nikic requested changes to this revision.Jun 10 2020, 3:00 AM
nikic added inline comments.
llvm/lib/Analysis/LazyValueInfo.cpp
1810

This will result in incorrect handling of pointers, as the miscompiles in non-null.ll show. This would need D69914 to be reapplied first. Additionally it doesn't really make sense to limit this to the entry block, this should be using the block value for everything (that would be D69686).

This revision now requires changes to proceed.Jun 10 2020, 3:00 AM
junparser marked an inline comment as done.Jun 10 2020, 4:18 AM

since we still need discuss about the invoke of getValueInBlock, I'll split this patch into two parts.

llvm/lib/Analysis/LazyValueInfo.cpp
1810

thanks for point this. I have not notice handling of pointers.
As for limit this to entry block. it is the same reason as you mentioned in D69686 (the different order of computing block value) which is not a issue in entry block.

fhahn added a comment.Jun 10 2020, 5:25 AM

since we still need discuss about the invoke of getValueInBlock, I'll split this patch into two parts.

I think this case could also be handled naturally by IPSCCP using PredicateInfo. Currently does only use range info from conditional branches, but I don't think there's any reason for not also using info from assume predicates, other than me not having time to implement it yet. If you want to give it a try, it would need to be added here: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Scalar/SCCP.cpp#L1243

since we still need discuss about the invoke of getValueInBlock, I'll split this patch into two parts.

I think this case could also be handled naturally by IPSCCP using PredicateInfo. Currently does only use range info from conditional branches, but I don't think there's any reason for not also using info from assume predicates, other than me not having time to implement it yet. If you want to give it a try, it would need to be added here: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Scalar/SCCP.cpp#L1243

Thanks for the info. I'll check later.

since we still need discuss about the invoke of getValueInBlock, I'll split this patch into two parts.

I think this case could also be handled naturally by IPSCCP using PredicateInfo. Currently does only use range info from conditional branches, but I don't think there's any reason for not also using info from assume predicates, other than me not having time to implement it yet. If you want to give it a try, it would need to be added here: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Scalar/SCCP.cpp#L1243

Thanks for the info. I'll check later.

Great, please feel free to reach out in case there are any questions.

junparser abandoned this revision.Aug 3 2020, 6:35 PM