Page MenuHomePhabricator

[BasicAA] Remove unnecessary known size requirement

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



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, but the problem was really with incorrect location sizes being provided in the first place (wrt handling of negative offsets), which was fixed by 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.Nov 14 2020, 10:13 AM
nikic requested review of this revision.Nov 14 2020, 10:13 AM
nikic planned changes to this revision.Nov 15 2020, 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 (, 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.Nov 17 2020, 4:46 PM
397 ↗(On Diff #305319)

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

nikic updated this revision to Diff 306722.EditedNov 20 2020, 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.Nov 24 2020, 3:05 PM
This revision was automatically updated to reflect the committed changes.