This is an archive of the discontinued LLVM Phabricator instance.

[Transforms] Propagate new-format TBAA tags on simplification of memory-transfer intrinsics
ClosedPublic

Authored by kosarev on Dec 22 2017, 4:39 AM.

Details

Summary

With this patch in place, when a new-format TBAA tag is available for a memory-transfer intrinsic call, we prefer propagating that new-format tag. Otherwise, we fallback to the old approach where we try to construct a proper TBAA access tag from 'tbaa.struct' metadata.

Diff Detail

Repository
rL LLVM

Event Timeline

kosarev created this revision.Dec 22 2017, 4:39 AM
kosarev edited the summary of this revision. (Show Details)Jan 8 2018, 6:46 AM

This seems reasonable, but I don't generally review LLVM patches.

Sure, I add you to reviewers of LLVM patches related the TBAA changes to make sure you are aware of the progress in this part of the work. If adding you to subscribers is preferable, I think that would work too. Just let me know. Thanks.

I applied this patch (target: i686-pc-windows-msvc) and run the test. No TBAA decorations present in the output. When running struct-assign-tbaa.ll from the same directory, the TBAA decorations are observed.

test/Transforms/InstCombine/struct-assign-tbaa-new.ll
42 ↗(On Diff #128015)

Pointer size depends on the platform. I would recommend you to add a target layout. See struct-assign-tbaa.ll for example.

This is due to new conventions for memory-transferring intrinsics introduced in rL322965. Will update in a moment. Thanks for reviewing.

kosarev updated this revision to Diff 134402.Feb 15 2018, 4:11 AM
  • Intrinsics calls updated to match the new conventions.
  • Added the target data layout string in the test.

TBAA decorations are still absent in the case of struct-assign-tbaa-new.ll, but it does not caused by this patch. The module returned by parseIRFile does not contain TBAA metadata in the case of this test, while struct-assign-tbaa.ll works as expected. Looks like an issue of loading new TBAA data from .ll file.

Hmm, somehow it works for me on r325320/Linux. Will re-check things and let you know then.

Still can't reproduce the problem. Just tried opt r325330 built on Windows 8.1 with MSVS Community 2017, and I can see the metadata in the output.

I used Visual Studio 2015 14.0.25425.01 Update 3. With it TBAA decorations do not appear in opt output. On the other host I installed a fresh copy of Visual Studio 2015, it has version 14.0.25431.01 Update 3. On that host the metadata appears and test passes.

sepavloff accepted this revision.Feb 19 2018, 12:03 AM

LGTM

As a possible further enhancement. memcpy accesses two memory locations, source and destination, so it is both a load and a store. Does it make sense decorate load access also?

This revision is now accepted and ready to land.Feb 19 2018, 12:03 AM

Separate tags for reading and writing aggregate accesses would certainly make sense. But that change would require some significant amount of time while being beyond the scope of the current work. This was briefly discussed before in D41539#966289.

This revision was automatically updated to reflect the committed changes.