This is an archive of the discontinued LLVM Phabricator instance.

[Analysis] Support aggregate access types in TBAA
ClosedPublic

Authored by kosarev on Dec 21 2017, 10:08 AM.

Details

Summary

This patch implements analysis for new-format TBAA access tags with aggregate types as their final access types.

Diff Detail

Repository
rL LLVM

Event Timeline

kosarev created this revision.Dec 21 2017, 10:08 AM

Hal, can you please review this patch?

hfinkel added inline comments.Jan 15 2018, 11:18 AM
lib/Analysis/TypeBasedAliasAnalysis.cpp
153 ↗(On Diff #127905)

Add a comment here reminding the reader that, in the old format, this first operand was a string.

231 ↗(On Diff #127905)

0 does not seem like a conservatively-correct result. I think this should either assert or return UINT64_MAX.

272 ↗(On Diff #127905)

This is the same as the function above. Maybe make a static helper function to avoid repeating the logic?

337 ↗(On Diff #127905)

Align this with the unsigned above.

555 ↗(On Diff #127905)

I think this should be UINT64_MAX, not 0, to be conservatively correct. IR generated by this code might be interpreted later by an implementation that does care about the sizes.

580 ↗(On Diff #127905)

Please document the meaning of this function's parameters and return value.

604 ↗(On Diff #127905)

Space after for.

608 ↗(On Diff #127905)

The assert should have a string that explains what's going on.

hfinkel accepted this revision.Feb 1 2018, 8:01 PM

A couple comments, but otherwise LGTM.

lib/Analysis/TypeBasedAliasAnalysis.cpp
640 ↗(On Diff #130397)

Please name this something other than BaseType because we already have a variable by that name in scope, and moreover, this is an access type (and so calling it 'BaseType' seems somewhat confusing (given that BaseType above is not necessarily an access type). Maybe just calling it BaseAccessType would be better.

Alternatively, just remove this because, to get here, I think that BaseType is always equal to the access type.

642 ↗(On Diff #130397)

Especially because, in the past, access types were only scalars, I think it would be useful to have a comment here reminding the reader than the access type might be an aggregate (thus explaining the need for this check).

This revision is now accepted and ready to land.Feb 1 2018, 8:01 PM
This revision was automatically updated to reflect the committed changes.