This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Fix non-determenistic behaviour
ClosedPublic

Authored by dnsampaio on Feb 4 2020, 8:22 AM.

Details

Summary

ARM Type Promotion pass does not clear
the container that defines if one variable
was visited or not, missing optimization
opportunities by luck when two llvm:Values
from different functions are allocated at
the same memory address.

Also fixes a comment and uses existing
method to pop and obtain last element
of the worklist.

Diff Detail

Event Timeline

dnsampaio created this revision.Feb 4 2020, 8:22 AM

Does the test work without debug info? Would be good to cut down the size of it.

Does the test work without debug info? Would be good to cut down the size of it.

Unfortunately no. With this file as is, around 10% of the time we obtain a failure. If I strip the debug information, it won't fail in 1000 runs.

dnsampaio edited the summary of this revision. (Show Details)Feb 4 2020, 9:13 AM
samparker accepted this revision.Feb 5 2020, 8:32 AM

Ok, well I guess for this pesky issue we can omit the test the. Please remove it before committing.

This revision is now accepted and ready to land.Feb 5 2020, 8:32 AM
dnsampaio updated this revision to Diff 242829.Feb 6 2020, 1:19 AM
  • Removed test
  • Added clear at the end of run as well, to clear waste
  • Moved clearing to a more sensible position
This revision was automatically updated to reflect the committed changes.

Do you think it's worth cherry-picking this to 10.0? CC @hans

Also, it looks like the test got dropped in the commit.

Do you think it's worth cherry-picking this to 10.0? CC @hans

Also, it looks like the test got dropped in the commit.

Hi @smeenai ,
The test drop was a reviewer request.
IMO, is a simple enough patch that should be cherry-picked.

hans added a comment.Feb 10 2020, 4:32 AM

Do you think it's worth cherry-picking this to 10.0? CC @hans

Also, it looks like the test got dropped in the commit.

Hi @smeenai ,
The test drop was a reviewer request.
IMO, is a simple enough patch that should be cherry-picked.

Sounds good to me. Cherry-picked to 10.x as a124bebdd5ff5cf49480956258c322ed9204943c