Page MenuHomePhabricator

Refine generation of TBAA information in clang
ClosedPublic

Authored by kosarev on Sep 13 2017, 1:52 PM.

Details

Summary

This patch is an attempt to clarify and simplify generation and propagation of TBAA information. The idea is to pack all values that describe a memory access, namely, base type, access type and offset, into a single structure. This is supposed to make further changes, such as adding support for unions and array members, easier to prepare and review.

Diff Detail

Repository
rL LLVM

Event Timeline

kosarev created this revision.Sep 13 2017, 1:52 PM
kosarev updated this revision to Diff 115183.Sep 14 2017, 1:38 AM
  • DecorateInstructionWithTBAA() is no more responsible for converting types to tags. These implicit conversions not only complicate reading the code, but also suggest assigning scalar access tags while we generally prefer full-size struct-path tags.
  • Removed unwanted stylistic changes.
hfinkel added inline comments.Sep 14 2017, 8:13 PM
lib/CodeGen/CGAtomic.cpp
1429 ↗(On Diff #115183)

You have a number of changes like this. AccessType is already a MDNode*, and I'm not sure why you're doing this. Will TBAA always equal LVal.getTBAAInfo().AccessType?

kosarev added inline comments.
lib/CodeGen/CGAtomic.cpp
1429 ↗(On Diff #115183)

Nope, it's not always the final access type that we want as the TBAA information.

Note that this snippet is functionally equivalent to the original piece. The difference is that the original code looks like we propagate the complete TBAA information (primarily of the misleading/outdated naming) while the patch makes it visible that we in fact decorate the atomic accesses with scalar tags that do not include any struct-path information. This is one of the reasons why the patched version looks more verbose. I am about to public an update to this patch that addresses this verbosity among other things.

kosarev updated this revision to Diff 115399.Sep 15 2017, 5:48 AM
  • TBAAAccessInfo is now capable of representing both scalar and struct-path accesses as well as the special cases like may-alias nodes and accesses to virtual table pointers.
  • Simplified the code that decorates instructions with TBAA information.
  • TBAAPathTag is replaced with TBAAAccessInfo; the latter is now the type of the keys of the cache map that translates access descriptors to metadata nodes.
  • Removed the scalar-specific cache map and related functions.
  • Decoration of atomic accesses is fixed to propagate the complete TBAA information and not just the final access types.
  • Fixed a bug with writing to a wrong map in getTBAABaseTypeMetadata() (former getTBAAStructTypeInfo()).
  • We now check for valid base access types every time we dereference a field. The original code only checks the top-level base type. See isValidTBAABaseType() / isTBAAPathStruct() calls.
  • Some entities have been renamed to sound more adequate and less confusing/misleading in presence of path-aware TBAA information.
  • Added description of the TBAAAccessInfo invariant.
  • Refined relevant comments and descriptions.
kosarev added inline comments.Sep 15 2017, 5:53 AM
lib/CodeGen/CodeGenTBAA.cpp
291 ↗(On Diff #115399)

Note the writing to StructMetadataCache instead of StructTypeMetadataCache. This is fixed in the patched version.

hfinkel added inline comments.Sep 15 2017, 7:05 PM
lib/CodeGen/CGExpr.cpp
3739 ↗(On Diff #115399)

This code has no dependence on mayAlias. Should it (if not for correctness, to avoid unnecessary work)? Maybe we should make the condition above:

if (TBAAPath && !mayAlias)
kosarev added inline comments.Sep 16 2017, 1:48 AM
lib/CodeGen/CGExpr.cpp
3739 ↗(On Diff #115399)

Right, we better handle the mayAlias case in a special way so we do not generate inappropriate access descriptors, even temporarily. And save some execution time too. Thanks Hal!

kosarev updated this revision to Diff 115533.Sep 16 2017, 2:35 AM
  • Handle may-alias base lvalues in a special way to address the note from Hal; see EmitLoadOfLValue().
  • Do not lookup twice for the same cache entry in getTBAAAccessMetadata().
  • Declare the single-argument TBAAAccessInfo constructor to be explicit to make the type-to-access conversions visible.

There are more TBAA patches on top of this change, of which the first is to make TBAA information to be part of LValueBaseInfo--all are steps toward the support for unions and member arrays. So friendly ping. Thanks.

kosarev edited reviewers, added: efriedma; removed: eli.friedman.Sep 22 2017, 4:17 AM

Colleagues, please let me know if I can do anything else to help with reviewing the patch. Thanks.

rjmccall edited edge metadata.Oct 3 2017, 6:38 PM

I assume I should wait on reviewing this until all of these smaller TBAA patches land?

kosarev updated this revision to Diff 117653.Oct 4 2017, 3:52 AM

Rebased.

This is how how the rebased patch differs from the mainline:

  • It incorporates changes from (not landed yet) D38503.
  • Simplifies generation of TBAA info in EmitLValueForField().
  • Replaces TBAAPathTag with TBAAAccessInfo for caching purposes.
  • Decorated atomic accesses with complete TBAA info and not just final access types (propagation of TBAA info for atomic accesses is already fixed in D38460).
  • Fixes the bug with writing to a wrong cache in getTBAAStructTypeInfo() / getBaseTypeInfo().

I assume I should wait on reviewing this until all of these smaller TBAA patches land?

These small patches are actually part of this diff. Generally, It depends on how you would like it: if you can review this re-based version as a whole, then that would save us some time.

Otherwise, you may want to proceed by smaller pieces, in which case D38503 is the next proposed piece to review.

Alternatively, if D38503 looks too complicated, please let me know and I will break it down into even smaller patches.

Or, if acceptable, I could commit a series of really small and simple diffs that all result in what is presented in D38126 and rely on post-commit reviews.

I'm fine with either way, provided the changes go to the mainline in time manner.

Thanks a lot for reviewing!

I assume I should wait on reviewing this until all of these smaller TBAA patches land?

These small patches are actually part of this diff. Generally, It depends on how you would like it: if you can review this re-based version as a whole, then that would save us some time.

Otherwise, you may want to proceed by smaller pieces, in which case D38503 is the next proposed piece to review.

Alternatively, if D38503 looks too complicated, please let me know and I will break it down into even smaller patches.

Or, if acceptable, I could commit a series of really small and simple diffs that all result in what is presented in D38126 and rely on post-commit reviews.

I'm fine with either way, provided the changes go to the mainline in time manner.

Thanks a lot for reviewing!

I'll let you rebase this on top of the D38503 when you commit it, and then I'll just review this directly. Thanks a lot for splitting it up!

John.

kosarev updated this revision to Diff 117823.Oct 5 2017, 8:17 AM
  • Rebased on top of D38503.
  • The tbaa-for-vptr.cpp test is changed to be more specific about the instructions to test. Otherwise, we fail on some triples, such as s390x-ibm-linux and x86_64-pc-mingw32. This is due to the changes for EmitLoadOfScalar() and EmitStoreOfScalar() where we previously were defaulting to empty TBAA info and now generate a TBAA descriptor based on the specified access type. This results in the instructions that load the pointer-to-function value having their TBAA tags (the may-alias one, namely) so that these instructions match the pattern in the test file and NUM gets a wrong value.
kosarev added inline comments.Oct 5 2017, 8:46 AM
lib/CodeGen/CodeGenTBAA.h
129 ↗(On Diff #117823)

Wrong function name. Will fix on next review cycle or commit.

rjmccall accepted this revision.Oct 5 2017, 8:02 PM

LGTM.

This revision is now accepted and ready to land.Oct 5 2017, 8:02 PM
This revision was automatically updated to reflect the committed changes.