Page MenuHomePhabricator

[BasicAA] Fix -basicaa-recphi for geps with negative offsets
ClosedPublic

Authored by dmgreen on Jul 10 2020, 10:33 AM.

Details

Summary

As shown in D82998, the basic-aa-recphi option can cause miscompiles for gep's with negative constants. The option checks for recursive phi, that recurse through a contant gep. If it finds one, it performs aliasing calculations using the other phi operands with an unknown size, to specify that an unknown number of elements after the initial value are potentially accessed. This works fine expect where the constant is negative, as the size is still considered to be positive. So this patch checks to make sure that the constant is also positive.

I will not attempt to turn the option back on until after the branch is made next week, to give us lots of time to catch anything else. But this should hopefully fix the issues.

Diff Detail

Event Timeline

dmgreen created this revision.Jul 10 2020, 10:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2020, 10:33 AM
hfinkel added inline comments.Fri, Jul 10, 2:00 PM
llvm/lib/Analysis/BasicAliasAnalysis.cpp
1686–1687

I would like to see the "The option checks for recursive phi, that recurse through a contant gep. If it finds one, it performs aliasing calculations using the other phi operands with an unknown size, to specify that an unknown number of elements after the initial value are potentially accessed. This works fine expect where the constant is negative, as the size is still considered to be positive. " part from the patch description -- that information -- in the comment here.

Also, can you make a lambda function do avoid the duplication of the comment and the condition in two places?

efriedma added inline comments.Fri, Jul 10, 2:12 PM
llvm/lib/Analysis/BasicAliasAnalysis.cpp
1689

Please avoid getSExtValue() when you can, in favor of APInt methods.

Do we need to check the GEP is inbounds?

dmgreen updated this revision to Diff 277241.Sat, Jul 11, 7:30 AM

Thanks. This creates a lambda, adds inbounds and isNegative, and updates some comments.

This revision is now accepted and ready to land.Sat, Jul 11, 12:31 PM
This revision was automatically updated to reflect the committed changes.

@xbolva00 This is an artifact I've noticed a few times. When this code in BasicAA is changed, there is a measurable compile-time impact even for changes that should be NFC. I suspect that it triggers some inlining heuristic in a visible way, but never looked into it. I don't think there's anything problematic in this commit itself (it changes functionality that is currently disabled.)