This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Decorate aggregate accesses with TBAA tags
ClosedPublic

Authored by kosarev on Dec 22 2017, 3:33 AM.

Diff Detail

Repository
rC Clang

Event Timeline

kosarev created this revision.Dec 22 2017, 3:33 AM
kosarev updated this revision to Diff 128017.Dec 22 2017, 5:07 AM

Reworked to not add another test file.

You're sure you just want a single TBAA tag on memcpy's? I would think that it might be useful to encode separate path information for the two operands.

Sure, but since it is not a trivial change we could first replace 'tbaa.struct' with 'tbaa' and then decide how to attach multiple TBAA tags to an instruction. I didn't think enough about it, but one way to do that is to have different kinds of TBAA tags for read and write accesses, in which case we could re-purpose the 'tbaa.struct' tag slot.

Yes, I think that's fine.

I think unconditionally attaching the tag is probably wrong; you need suppress it in the may_alias cases. I'll grant that the existing code doesn't honor that, but that seems like a bug we should go ahead and fix. You'll need to either propagate more information into this function or propagate out the memcpy instruction so that callers can decide how to apply TBAA.

OK, I'm reading your response so that this patch may significantly increase the number of cases where we propagate TBAA tags from memory-transfer intrinsic calls, which means potentially more cases where ignoring may_alias would lead to problems. If so, then I tend to agree. Will prepare another version. Thanks.

kosarev updated this revision to Diff 130599.Jan 19 2018, 6:13 AM
  • Supported propagation of TBAA info for aggregate transfers.
  • Added tests on copying of may_alias aggregates.

Thank you. Maybe there should be an overload of EmitAggregateCopy that takes LValues? A lot of these cases start with an LValue on at least one side, and there are already some convenience functions to turn an Address into a naturally-typed LValue.

kosarev updated this revision to Diff 131257.Jan 24 2018, 6:48 AM

The copying functions changed to take LValues. It seems Address-taking versions are not very useful so we don't bother with overloading.

rjmccall accepted this revision.Jan 24 2018, 3:46 PM

Okay, that works, thanks! LGTM.

This revision is now accepted and ready to land.Jan 24 2018, 3:46 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.