Page MenuHomePhabricator

[AliasAnalysis] Fix a bug in alias analysis interaction with CSE w/ memorySSA
AbandonedPublic

Authored by haoranxu510 on Oct 22 2020, 3:44 PM.

Details

Summary

I noticed that clang miscompiled a C code generated from brainfxxk-to-C transpiler
(https://godbolt.org/z/676Grr), under -O1 or higher, a loop is wrongly optimized
into a dead loop.

After investigation, it turned out that the cause of the issue is that CSE w/
memorySSA transformation pass wrongly eliminated a --(*ptr) in the loop using
the result of another --(*ptr) outside the loop, without realizing that *ptr
may have been modified by the loop.

By bisecting the git history, it turned out that the bug was introduced by D59315.
The said diff seems to be a refactoring diff, however, it seems like the author
accidentally removed two lines of logic in refactoring, resulting in the bug.

I added the two lines back. While it solves the correctness issue, I'm not certain
on the performance implications of this change, since by clearing the AAQI after
each alias query, perhaps we would lose most of the gain from batching AA queries.

During the investigation, I also noticed that the author forgot to change a few
places to use his new API, however, I'm not sure if it is a correctness issue
or not.

Diff Detail

Event Timeline

haoranxu510 created this revision.Oct 22 2020, 3:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2020, 3:44 PM
haoranxu510 requested review of this revision.Oct 22 2020, 3:44 PM
haoranxu510 edited the summary of this revision. (Show Details)Oct 22 2020, 3:47 PM

Needs a test

Hi Roman,

Thanks for your comment!
I'm new to LLVM, can you explain what test would be needed? So currently I have the C code snippet that is miscompiled before this diff and works after this diff, however, the snippet is ~100KB. Do you think it is OK to add a test like that?
I don't know the internals of LLVM so I don't know how I can construct a not-end-to-end failing case.

asbirlea requested changes to this revision.Oct 29 2020, 6:10 PM

I checked in the changes adding AAQI as authored by you.
The change clearing the cache is not right, as discussed on llvm-dev, so I think this patch is not obsolete.

This revision now requires changes to proceed.Oct 29 2020, 6:10 PM
haoranxu510 abandoned this revision.Oct 29 2020, 8:05 PM

Thanks!