This is an archive of the discontinued LLVM Phabricator instance.

Remove alignment argument from memcpy/memmove/memset in favour of alignment attributes (Step 1)
ClosedPublic

Authored by dneilson on Jan 2 2018, 11:58 AM.

Details

Summary

This is a resurrection of work first proposed and discussed in Aug 2015:

http://lists.llvm.org/pipermail/llvm-dev/2015-August/089384.html

and initially landed (but then backed out) in Nov 2015:

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

The @llvm.memcpy/memmove/memset intrinsics currently have an explicit argument
which is required to be a constant integer. It represents the alignment of the
dest (and source), and so must be the minimum of the actual alignment of the
two.

This change is the first in a series that allows source and dest to each
have their own alignments by using the alignment attribute on their arguments.

In this change we:

  1. Remove the alignment argument.
  2. Add alignment attributes to the source & dest arguments. We, temporarily, require that the alignments for source & dest be equal.

    For example, code which used to read: call void @llvm.memcpy.p0i8.p0i8.i32(i8* %dest, i8* %src, i32 100, i32 4, i1 false)

will now read

call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 %dest, i8* align 4 %src, i32 100, i1 false)

Downstream users may have to update their lit tests that check for
@llvm.memcpy/memmove/memset call/declaration patterns. The following extended sed script
may help with updating the majority of your tests, but it does not catch all possible
patterns so some manual checking and updating will be required.

s~declare void @llvm\.mem(set|cpy|move)\.p([^(]*)\((.*), i32, i1\)~declare void @llvm.mem\1.p\2(\3, i1)~g
s~call void @llvm\.memset\.p([^(]*)i8\(i8([^*]*)\* (.*), i8 (.*), i8 (.*), i32 [01], i1 ([^)]*)\)~call void @llvm.memset.p\1i8(i8\2* \3, i8 \4, i8 \5, i1 \6)~g
s~call void @llvm\.memset\.p([^(]*)i16\(i8([^*]*)\* (.*), i8 (.*), i16 (.*), i32 [01], i1 ([^)]*)\)~call void @llvm.memset.p\1i16(i8\2* \3, i8 \4, i16 \5, i1 \6)~g
s~call void @llvm\.memset\.p([^(]*)i32\(i8([^*]*)\* (.*), i8 (.*), i32 (.*), i32 [01], i1 ([^)]*)\)~call void @llvm.memset.p\1i32(i8\2* \3, i8 \4, i32 \5, i1 \6)~g
s~call void @llvm\.memset\.p([^(]*)i64\(i8([^*]*)\* (.*), i8 (.*), i64 (.*), i32 [01], i1 ([^)]*)\)~call void @llvm.memset.p\1i64(i8\2* \3, i8 \4, i64 \5, i1 \6)~g
s~call void @llvm\.memset\.p([^(]*)i128\(i8([^*]*)\* (.*), i8 (.*), i128 (.*), i32 [01], i1 ([^)]*)\)~call void @llvm.memset.p\1i128(i8\2* \3, i8 \4, i128 \5, i1 \6)~g
s~call void @llvm\.memset\.p([^(]*)i8\(i8([^*]*)\* (.*), i8 (.*), i8 (.*), i32 ([0-9]*), i1 ([^)]*)\)~call void @llvm.memset.p\1i8(i8\2* align \6 \3, i8 \4, i8 \5, i1 \7)~g
s~call void @llvm\.memset\.p([^(]*)i16\(i8([^*]*)\* (.*), i8 (.*), i16 (.*), i32 ([0-9]*), i1 ([^)]*)\)~call void @llvm.memset.p\1i16(i8\2* align \6 \3, i8 \4, i16 \5, i1 \7)~g
s~call void @llvm\.memset\.p([^(]*)i32\(i8([^*]*)\* (.*), i8 (.*), i32 (.*), i32 ([0-9]*), i1 ([^)]*)\)~call void @llvm.memset.p\1i32(i8\2* align \6 \3, i8 \4, i32 \5, i1 \7)~g
s~call void @llvm\.memset\.p([^(]*)i64\(i8([^*]*)\* (.*), i8 (.*), i64 (.*), i32 ([0-9]*), i1 ([^)]*)\)~call void @llvm.memset.p\1i64(i8\2* align \6 \3, i8 \4, i64 \5, i1 \7)~g
s~call void @llvm\.memset\.p([^(]*)i128\(i8([^*]*)\* (.*), i8 (.*), i128 (.*), i32 ([0-9]*), i1 ([^)]*)\)~call void @llvm.memset.p\1i128(i8\2* align \6 \3, i8 \4, i128 \5, i1 \7)~g
s~call void @llvm\.mem(cpy|move)\.p([^(]*)i8\(i8([^*]*)\* (.*), i8([^*]*)\* (.*), i8 (.*), i32 [01], i1 ([^)]*)\)~call void @llvm.mem\1.p\2i8(i8\3* \4, i8\5* \6, i8 \7, i1 \8)~g
s~call void @llvm\.mem(cpy|move)\.p([^(]*)i16\(i8([^*]*)\* (.*), i8([^*]*)\* (.*), i16 (.*), i32 [01], i1 ([^)]*)\)~call void @llvm.mem\1.p\2i16(i8\3* \4, i8\5* \6, i16 \7, i1 \8)~g
s~call void @llvm\.mem(cpy|move)\.p([^(]*)i32\(i8([^*]*)\* (.*), i8([^*]*)\* (.*), i32 (.*), i32 [01], i1 ([^)]*)\)~call void @llvm.mem\1.p\2i32(i8\3* \4, i8\5* \6, i32 \7, i1 \8)~g
s~call void @llvm\.mem(cpy|move)\.p([^(]*)i64\(i8([^*]*)\* (.*), i8([^*]*)\* (.*), i64 (.*), i32 [01], i1 ([^)]*)\)~call void @llvm.mem\1.p\2i64(i8\3* \4, i8\5* \6, i64 \7, i1 \8)~g
s~call void @llvm\.mem(cpy|move)\.p([^(]*)i128\(i8([^*]*)\* (.*), i8([^*]*)\* (.*), i128 (.*), i32 [01], i1 ([^)]*)\)~call void @llvm.mem\1.p\2i128(i8\3* \4, i8\5* \6, i128 \7, i1 \8)~g
s~call void @llvm\.mem(cpy|move)\.p([^(]*)i8\(i8([^*]*)\* (.*), i8([^*]*)\* (.*), i8 (.*), i32 ([0-9]*), i1 ([^)]*)\)~call void @llvm.mem\1.p\2i8(i8\3* align \8 \4, i8\5* align \8 \6, i8 \7, i1 \9)~g
s~call void @llvm\.mem(cpy|move)\.p([^(]*)i16\(i8([^*]*)\* (.*), i8([^*]*)\* (.*), i16 (.*), i32 ([0-9]*), i1 ([^)]*)\)~call void @llvm.mem\1.p\2i16(i8\3* align \8 \4, i8\5* align \8 \6, i16 \7, i1 \9)~g
s~call void @llvm\.mem(cpy|move)\.p([^(]*)i32\(i8([^*]*)\* (.*), i8([^*]*)\* (.*), i32 (.*), i32 ([0-9]*), i1 ([^)]*)\)~call void @llvm.mem\1.p\2i32(i8\3* align \8 \4, i8\5* align \8 \6, i32 \7, i1 \9)~g
s~call void @llvm\.mem(cpy|move)\.p([^(]*)i64\(i8([^*]*)\* (.*), i8([^*]*)\* (.*), i64 (.*), i32 ([0-9]*), i1 ([^)]*)\)~call void @llvm.mem\1.p\2i64(i8\3* align \8 \4, i8\5* align \8 \6, i64 \7, i1 \9)~g
s~call void @llvm\.mem(cpy|move)\.p([^(]*)i128\(i8([^*]*)\* (.*), i8([^*]*)\* (.*), i128 (.*), i32 ([0-9]*), i1 ([^)]*)\)~call void @llvm.mem\1.p\2i128(i8\3* align \8 \4, i8\5* align \8 \6, i128 \7, i1 \9)~g

The remaining changes in the series will:
Step 2) Expand the IRBuilder API to allow creation of memcpy/memmove with differing

source and dest alignments.

Step 3) Update Clang to use the new IRBuilder API.
Step 4) Update Polly to use the new IRBuilder API.
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
getDestAlignment() and getSourceAlignment() instead.

Step 6) Remove the single-alignment IRBuilder API for memcpy/memmove, and the

MemIntrinsicInst::[get|set]Alignment() methods.

Diff Detail

Repository
rL LLVM

Event Timeline

dneilson created this revision.Jan 2 2018, 11:58 AM
dneilson updated this revision to Diff 128468.Jan 2 2018, 2:09 PM
  • Running the sed script changed some test files only by the addition of a newline at the end of the file. Undoing these changes to reduce the number of files touched by this change.
arsenm added inline comments.Jan 2 2018, 2:14 PM
lib/IR/AutoUpgrade.cpp
526 ↗(On Diff #128468)

Is there a point to changing the name to something that will just be deleted?

dneilson added inline comments.Jan 2 2018, 2:28 PM
lib/IR/AutoUpgrade.cpp
526 ↗(On Diff #128468)

Good question. This was in the original November 2015 change, and I just blindly copied it over.

I just tried removing the setName() calls, and am getting an assertion failure in the LLVM unit-test Linker/./LinkerTests/LinkModuleTest.RemangleIntrinsics:

Assertion failed: (isa<X>(Val) && "cast<Ty>() argument of incompatible type!"), function cast, file /Users/dneilson/Development/LLVM-dev/llvm/include/llvm/Support/Casting.h, line 255.
...
8  LinkerTests              0x000000010ab03197 llvm::cast_retty<llvm::Function, llvm::Constant*>::ret_type llvm::cast<llvm::Function, llvm::Constant>(llvm::Constant*) + 103
9  LinkerTests              0x000000010ac0b72e llvm::Intrinsic::getDeclaration(llvm::Module*, llvm::Intrinsic::ID, llvm::ArrayRef<llvm::Type*>) + 734
10 LinkerTests              0x000000010aaa28b8 UpgradeIntrinsicFunction1(llvm::Function*, llvm::Function*&) + 21096
11 LinkerTests              0x000000010aa9d578 llvm::UpgradeIntrinsicFunction(llvm::Function*, llvm::Function*&) + 40
12 LinkerTests              0x000000010aac953b llvm::UpgradeCallsToIntrinsic(llvm::Function*) + 107
13 LinkerTests              0x000000010a9bce8e llvm::LLParser::ValidateEndOfModule() + 10638

I'm guessing that this is updating a declaration, and doesn't actually delete the declaration that it's replacing right away. So, renaming to append ".old" to the name effectively gets it out of the way and allows it to be deleted later on.

ping -- Anyone able to review?

arsenm added inline comments.Jan 10 2018, 8:14 AM
lib/IR/AutoUpgrade.cpp
526 ↗(On Diff #128468)

It looks like this is how at least some places do this, but use the rename() function. This should probably do the same

dneilson marked an inline comment as done.Jan 10 2018, 8:26 AM
dneilson added inline comments.
lib/IR/AutoUpgrade.cpp
526 ↗(On Diff #128468)

Easy enough to change. Thanks for following up.

reames requested changes to this revision.Jan 10 2018, 3:12 PM
reames added a subscriber: reames.

Meta comment: This change is too large *conceptually*. You need to break it apart in a way that the test changes are "obviously correct" because there's no possible change in behaviour. At the moment, it a behaviour change might be lurking in the test changes and that'd never be seen.

My suggest approach would be to do this in multiple phases:

  1. separate out the Attribute removal changes, unit test via C++
  2. change how the alignment is represented (separate argument to param attributes), but add a temporary verifier rule that the source and dest have the same alignment. Update the set functions to update both, and the read to take one of the at random (because they're equal). No client code should change! But this will include all of your *NFC* test updates.
  3. Remove the temporary verifier rule and split the alignment API as appropriate. Should have functional tests for refined behaviour (i.e. able to strengthen only one param). Likely can generalize some existing handling to any align param attribute.
  4. Land any NFC renames you desire (i.e. memset Align to DestAlign). These should not be intermixed with preceding changes!
docs/LangRef.rst
10366 ↗(On Diff #128468)

I might add a bit about the expected performance impact of adding align.

10382 ↗(On Diff #128468)

You can drop this. The same information can be found by using the older docs directly.

10456 ↗(On Diff #128468)

Same

10525 ↗(On Diff #128468)

Same.

include/llvm/IR/IRBuilder.h
424 ↗(On Diff #128468)

To split the change, you could leave the Align parameter in place, and set both to the alignment. Anyone who wanted to split them could directly access the parameter attributes on the resulting call.

include/llvm/IR/IntrinsicInst.h
400 ↗(On Diff #128468)

You could break up the patch by leaving these in place as cover functions. You could do the following:

  1. setAlignment sets alignment on both params
  2. getAlignment returns the minimum of the two
472 ↗(On Diff #128468)

You can and should separate and land this NFC change.

lib/CodeGen/SafeStack.cpp
567 ↗(On Diff #128468)

It's not obvious Align is correct here. Please match the old behaviour and improve in a followon patch. (i.e. pass the arg alignment to both)

lib/CodeGen/SafeStackLayout.cpp
45 ↗(On Diff #128468)

Isn't this already available in the other structure?

lib/IR/Attributes.cpp
546 ↗(On Diff #128468)

Huh? Is this a separate bug fix? If so, separate. Otherwise, why is it in this patch?

This revision now requires changes to proceed.Jan 10 2018, 3:12 PM
dneilson updated this revision to Diff 129550.Jan 11 2018, 4:11 PM
dneilson marked an inline comment as done.

Refactor change.

This is now step 1 of 6 in the path to fully moving memcpy/memmove/memset over to align attributes.

Will update the description to match.

dneilson retitled this revision from Change memcpy/memove/memset to have dest and source alignment attributes. to Remove alignment argument from memcpy/memmove/memset in favour of alignment attributes (Step 1).Jan 11 2018, 4:12 PM
dneilson edited the summary of this revision. (Show Details)

I pulled the fix to AttributeSet/AttributeList into its own change; see parent of this change.

reames requested changes to this revision.Jan 12 2018, 9:22 AM

Much closer to reasonable, likely one or two more iterations.

include/llvm/IR/IntrinsicInst.h
428 ↗(On Diff #129550)

Why is this not just a auto *this_MTI = cast<MemTransferInst>(this)?

I'm fine with this spelling, just point out the alternative.

452 ↗(On Diff #129550)

This ones a lot more awkward and the other idiom would be preferred.

lib/CodeGen/CodeGenPrepare.cpp
1613 ↗(On Diff #129550)

You can and should land a refactoring without further review which does the following:

  1. add a cover function setAlignment(unsigned) which internally creates the constant and passses it in
  2. update the various callers (like this one) to use it

Doing so will remove this file entirely from the patch.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5013 ↗(On Diff #129550)

Changes in this file (removing the raw attribute indexes) can be done on the existing API and landed without further review. Please include the TODOs.

lib/IR/AutoUpgrade.cpp
2401 ↗(On Diff #129550)

This file needs careful review. I'll do it as a last resort, but if there's someone familiar with this code who wants to step in...

I'm deferring review on this file until everything else has settled.

lib/IR/IRBuilder.cpp
81 ↗(On Diff #129550)

I think this is just whitespace? If so, remove!

lib/Transforms/InstCombine/InstCombineCalls.cpp
194 ↗(On Diff #129550)

As mentioned previously, use a cover function, separate and land.

lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
1388 ↗(On Diff #129550)

Again, cover function.

lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
387 ↗(On Diff #129550)

Again, cover function.

lib/Transforms/Scalar/SROA.cpp
2809 ↗(On Diff #129550)

Cover function.

This revision now requires changes to proceed.Jan 12 2018, 9:22 AM
dneilson updated this revision to Diff 129763.Jan 13 2018, 7:59 AM
dneilson edited the summary of this revision. (Show Details)
  • Addressing review comments.
reames accepted this revision.Jan 17 2018, 6:30 PM

LGTM w/minor comment applied.

lib/IR/IRBuilder.cpp
126 ↗(On Diff #129763)

very minor:
if (Align > 0)

cast<MemCpyInst>(CI)->setAlignment(Align);
This revision is now accepted and ready to land.Jan 17 2018, 6:30 PM
dneilson updated this revision to Diff 130416.Jan 18 2018, 7:59 AM
  • Address the minor comment.
  • Rebase.
  • clang-format
This revision was automatically updated to reflect the committed changes.