Page MenuHomePhabricator

[BasicAA] Remove unnecessary known size requirement
ClosedPublic

Authored by nikic on Sat, Nov 14, 10:13 AM.

Details

Summary

This is tangentially related to D91383: After that patch, the offset/size conditions for the variable index case are the same as for the no-variable case, with one exception: If there are no variables and the base offset is negative, we also check that V2 doesn't have unknown size.

I believe this check is not necessary. It was introduced in https://github.com/llvm/llvm-project/commit/e3ac099726571b58333b0fee827831d420d24285, but the problem was really with incorrect location sizes being provided in the first place (wrt handling of negative offsets), which was fixed by https://github.com/llvm/llvm-project/commit/1726fc698ccb85fe4bb23c200a50f28b57fc53cb. Unknown location sizes are still required to be non-negative, so the second access cannot start below zero.

I've added a test using a store and a memset to illustrate a case where this makes a difference.

Diff Detail

Event Timeline

nikic created this revision.Sat, Nov 14, 10:13 AM
nikic requested review of this revision.Sat, Nov 14, 10:13 AM
nikic planned changes to this revision.Sun, Nov 15, 11:53 AM

While I believe this change is correct, there are some incorrect AA uses that should be addressed first. There's one in MemorySSA wrt phi translation over decreasing cycles (https://reviews.llvm.org/D87778#inline-852179), and another in ParallelDSP (see test failure).

I think we need to clarify the meaning of LocationSize::unknown() in doc comments, and add a variant of it that means "unknown including negative sizes". The standard pattern for alias() queries is to simply pass both sizes as unknown if you want that behavior, but for getModRefInfo this trick doesn't work.

asbirlea added inline comments.Tue, Nov 17, 4:46 PM
llvm/test/Analysis/MemorySSA/phi-translation.ll
397 ↗(On Diff #305319)

This doesn't look right. The address loaded is modified in the loop.

nikic updated this revision to Diff 306722.EditedFri, Nov 20, 9:58 AM
nikic edited the summary of this revision. (Show Details)

Rebase over D91649, after which the correctness of this change should be more obvious. At this point in BasicAA, all LocationSizes only allow access after the base pointer.

The changes to MemorySSA tests are now gone.

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