Page MenuHomePhabricator

[BasicAA] Fix BatchAA results for phi-phi assumptions
Changes PlannedPublic

Authored by nikic on Sun, Nov 22, 10:58 AM.

Details

Summary

Add a flag that disables caching when computing aliasing results potentially based on a phi-phi NoAlias assumption. We'll still insert cache entries temporarily to catch infinite recursion, but will drop them afterwards, so they won't persist in BatchAA.

Diff Detail

Event Timeline

nikic created this revision.Sun, Nov 22, 10:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptSun, Nov 22, 10:58 AM
nikic requested review of this revision.Sun, Nov 22, 10:58 AM
nikic updated this revision to Diff 306928.EditedSun, Nov 22, 11:34 AM
nikic edited the summary of this revision. (Show Details)

Rebase. I've committed the not-really-related change that lead to the compile-time improvement separately. With that gone, the new numbers are: https://llvm-compile-time-tracker.com/compare.php?from=6f5ef648a57aed3274118dcbd6467e8ac4f6ed1f&to=50d73a31620111c4298cc336ac4682ef7585ed6b&stat=instructions Which does make this a mild compile-time regression, but not too bad.

asbirlea accepted this revision.Tue, Nov 24, 4:29 PM

This is great, thank you for this fix!

This revision is now accepted and ready to land.Tue, Nov 24, 4:29 PM
This revision was automatically updated to reflect the committed changes.

Which does make this a mild compile-time regression, but not too bad.

I've hit a case where this causes compilation to hang indefinitely (or so it seems; the case earlier compiled in around 2 seconds, and now hasn't finished after at least 7 minutes), with https://martin.st/temp/dvdec-preproc.c compiled with clang -target x86_64-w64-mingw32 -w -c -O3 dvdec-preproc.c.

nikic reopened this revision.Fri, Nov 27, 1:35 PM

@mstorsjo Thanks for the report & revert.

Best reduction for the hang I have for now is https://gist.github.com/nikic/bc0e46b99555c93995d07c55cb4f8420 under -O3.

This revision is now accepted and ready to land.Fri, Nov 27, 1:35 PM

We also found an infinite loop triggered by this. Our repro is at https://bugs.chromium.org/p/chromium/issues/detail?id=1153398#c3

nikic planned changes to this revision.Sun, Nov 29, 1:38 AM

This will clearly need something more sophisticated. After thinking this over, I also think that the current approach of inserting assumptions at phis is not optimal, as the tests added in https://github.com/llvm/llvm-project/commit/b5e8de9c7903d458b098a8af03939384270c1a5e show. I expect this can also result in BatchAA returning different results, depending on whether a gep or a phi is visited first. We should be inserting everything as NoAlias into the cache in the first place and tracking whether that assumption gets used or not.