Page MenuHomePhabricator

[SROA] Take advantage of separate alignments for memcpy source and destination
ClosedPublic

Authored by dneilson on Feb 6 2018, 10:46 AM.

Details

Summary

This change is part of step five in the series of changes to remove alignment argument from
memcpy/memmove/memset in favour of alignment attributes. In particular, this changes the
SROA pass to cease using the old getAlignment() & setAlignment() APIs of MemoryIntrinsic in
favour of getting source & dest specific alignments through the new API. This allows us
to enhance visitMemTransferInst to be more aggressive setting the alignment in memcpy
calls that it creates, as well as to only change the alignment of a memcpy/memmove
argument that it replaces.

Steps:
Step 1) Remove alignment parameter and create alignment parameter attributes for
memcpy/memmove/memset. ( rL322965, rC322964, rL322963 )
Step 2) Expand the IRBuilder API to allow creation of memcpy/memmove with differing
source and dest alignments. ( rL323597 )
Step 3) Update Clang to use the new IRBuilder API. ( rC323617 )
Step 4) Update Polly to use the new IRBuilder API. ( rL323618 )
Step 5) Update LLVM passes that create memcpy/memmove calls to use the new IRBuilder API,
and those that use use MemIntrinsicInst::[get|set]Alignment() to use [get|set]DestAlignment()
and [get|set]SourceAlignment() instead. ( rL323886, rL323891, rL324148, rL324273 )
Step 6) Remove the single-alignment IRBuilder API for memcpy/memmove, and the
MemIntrinsicInst::[get|set]Alignment() methods.

Reference

http://lists.llvm.org/pipermail/llvm-dev/2015-August/089384.html
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20151109/312083.html

Diff Detail

Repository
rL LLVM

Event Timeline

dneilson created this revision.Feb 6 2018, 10:46 AM
dneilson updated this revision to Diff 135460.Feb 22 2018, 10:11 AM
  • Rebaseline

ping. Anyone able to review?

efriedma added a subscriber: efriedma.
efriedma added inline comments.
lib/Transforms/Scalar/SROA.cpp
2701 ↗(On Diff #135460)

Testcase?

2828 ↗(On Diff #135460)

Testcase? (At least, at first glance it doesn't look like the memcpy tests cover this.)

2899 ↗(On Diff #135460)

All the checks for IsDest here are confusing... could you make explicit variables SrcPtr/DstPtr/SrcAlign/DstAlign?

Thank you for taking a look at this Eli! Very much appreciated

lib/Transforms/Scalar/SROA.cpp
2701 ↗(On Diff #135460)

Any number of the memsets in tests/Transforms/SROA/basictest.ll act as a test for this. For example, the memset in test18 at line 783.

2828 ↗(On Diff #135460)

test18 -- the memcpy at line 798 tests the IsDest case
test18 -- the memcpy at line 803 tests the !IsDest case

There are others as well; this is just a sampling.

2899 ↗(On Diff #135460)

Fair point.. I completely agree. I'll get a revision up shortly.

efriedma added inline comments.Mar 12 2018, 2:56 PM
lib/Transforms/Scalar/SROA.cpp
2701 ↗(On Diff #135460)

This isn't actually a functional change; nevermind.

2828 ↗(On Diff #135460)

Okay. It would be nice to have a test which explicitly checks we preserve the alignment on the other operand.

dneilson added inline comments.Mar 12 2018, 3:06 PM
lib/Transforms/Scalar/SROA.cpp
2828 ↗(On Diff #135460)

Those same memcpys end up testing both; that's why the test case change in test18 is as it is in this patch. I'll annotate the lines in the test below...

test/Transforms/SROA/basictest.ll
83 ↗(On Diff #135460)

FYI - the alignment on the %src arg here was changed to force some variety in the alignments on memcpys that are created in this test.

781 ↗(On Diff #135460)

This is the IsDest case for !IsSplittable

790 ↗(On Diff #135460)

This is the !IsDest case for !IsSplittable

dneilson updated this revision to Diff 138096.Mar 12 2018, 3:08 PM

Rebaseline & address review comment regarding reducing the usage of ternary operator
in creation of memcpy.

This revision is now accepted and ready to land.Mar 12 2018, 3:34 PM
This revision was automatically updated to reflect the committed changes.