Page MenuHomePhabricator

[FuncSpec]Fix assertion failure when lattice value is not found.
ClosedPublic

Authored by sinan on May 25 2022, 12:52 AM.

Details

Summary

The patch from D110529 partially solves the problem of bug51600 . However, the reason for crashing is that if an argument is byval, and if the function is not readonly, 'ValueState' will not record the argument(in SCCPInstVisitor::handleCallArguments), and thus we cannot find its value state (in getPossibleConstants) and this leads to an assertation failure or crash.

The problem still exists in the current codebase, and this patch can solve it and also relax the constraint on constant expression. The attached test cases *-expression4.ll and *-expression5.ll can bypass the check of global variable and constant expression and trigger assertation failure.

Diff Detail

Event Timeline

sinan created this revision.May 25 2022, 12:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2022, 12:52 AM
sinan requested review of this revision.May 25 2022, 12:52 AM

We should generate a patch with option -U9999 to offer more context to review.

Thanks for working on this. Have you measured any performance/code size change?

Also, I feel we could do better since we've already mark it as overrefined in https://github.com/llvm/llvm-project/blob/cd2292ef824591cc34cc299910a3098545c840c7/llvm/lib/Transforms/Utils/SCCPSolver.cpp#L1254
So it looks possible to get the lattice successfully.

sinan updated this revision to Diff 432181.May 25 2022, 8:19 PM
sinan edited the summary of this revision. (Show Details)
sinan added a comment.May 25 2022, 8:37 PM

We should generate a patch with option -U9999 to offer more context to review.

Thanks for the info and feedback. I have updated the diff file.

Thanks for working on this. Have you measured any performance/code size change?

I only ran on Spec 2017, and luckily no crash during compiling. I will check the performance/code size different later.

Also, I feel we could do better since we've already mark it as overrefined in https://github.com/llvm/llvm-project/blob/cd2292ef824591cc34cc299910a3098545c840c7/llvm/lib/Transforms/Utils/SCCPSolver.cpp#L1254
So it looks possible to get the lattice successfully.

Ok, I will think about it, thanks again!

We should generate a patch with option -U9999 to offer more context to review.

Thanks for the info and feedback. I have updated the diff file.

Thanks for working on this. Have you measured any performance/code size change?

I only ran on Spec 2017, and luckily no crash during compiling. I will check the performance/code size different later.

Also, I feel we could do better since we've already mark it as overrefined in https://github.com/llvm/llvm-project/blob/cd2292ef824591cc34cc299910a3098545c840c7/llvm/lib/Transforms/Utils/SCCPSolver.cpp#L1254
So it looks possible to get the lattice successfully.

Ok, I will think about it, thanks again!

Given it might be non-trivial to support ByVal arguments in SCCPSolver, and ByVal is generally used for pointers to structure/array, we can first try to get this version of patch reviewed and merged, then work on ByVal/SCCPSolver as a following task (probably with a lower priority).

sinan added a comment.EditedJun 1 2022, 10:24 PM

We should generate a patch with option -U9999 to offer more context to review.

Thanks for working on this. Have you measured any performance/code size change?

Also, I feel we could do better since we've already mark it as overrefined in https://github.com/llvm/llvm-project/blob/cd2292ef824591cc34cc299910a3098545c840c7/llvm/lib/Transforms/Utils/SCCPSolver.cpp#L1254
So it looks possible to get the lattice successfully.

Hi,

I tested with flag -Wl,-mllvm,--enable-function-specialization on Spec 2017. No noticeable regression in compilation time(measured by -fproc-stat-report) and performance was found.

ChuanqiXu accepted this revision.Jun 5 2022, 7:14 PM

LGTM then.

This revision is now accepted and ready to land.Jun 5 2022, 7:14 PM
This revision was landed with ongoing or failed builds.Jun 10 2022, 3:46 AM
This revision was automatically updated to reflect the committed changes.