Page MenuHomePhabricator

[InstCombine] Simplify MemTransferInst's source and dest alignments separately
ClosedPublic

Authored by dneilson on Feb 2 2018, 3:03 PM.

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
InstCombine pass to cease using the deprecated MemoryIntrinsic::getAlignment() method, and
instead we use the separate getSourceAlignment and getDestAlignment APIs to simplify
the source and destination alignment attributes separately.

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, r323891 )
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 2 2018, 3:03 PM
efriedma added inline comments.
lib/Transforms/InstCombine/InstCombineCalls.cpp
245 ↗(On Diff #132691)

Aren't SrcAlign and CopySrcAlign always the same?

dneilson added inline comments.Feb 12 2018, 12:23 PM
lib/Transforms/InstCombine/InstCombineCalls.cpp
245 ↗(On Diff #132691)

I'm guessing that the original code was there because it's possible for there to be a greater alignment value on the memory intrinsic than getKnownAlignment() would give us. I'm not really sure how that would happen.... I suppose that it's possible that now that the alignment on the memory intrinsic isn't just the min of the two arg alignments that these lines are now redundant.

efriedma added inline comments.Feb 12 2018, 12:43 PM
lib/Transforms/InstCombine/InstCombineCalls.cpp
245 ↗(On Diff #132691)

I'm not really sure how that would happen....

In the old world, SrcAlign could be larger than CopyAlign if the dest has low/unknown alignment. And CopyAlign could be larger than SrcAlign if the frontend or some other pass computed better alignment than getKnownAlignment.

In the new world, CopySrcAlign is always greater than or equal to SrcAlign; you explicitly check it a few lines earlier.

dneilson updated this revision to Diff 133927.Feb 12 2018, 1:54 PM
  • Was being stupid. We can just use the alignment from the memory intrinsic on the materialized loads/stores, so do that.
This revision is now accepted and ready to land.Feb 12 2018, 2:34 PM
This revision was automatically updated to reflect the committed changes.