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

[dneilson]

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.

[dneilson]

• 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]

lib/IR/AutoUpgrade.cpp

526 ↗

(On Diff #128468)

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

[dneilson]

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.

[dneilson]

ping -- Anyone able to review?

[arsenm]

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]

lib/IR/AutoUpgrade.cpp

526 ↗

(On Diff #128468)

Easy enough to change. Thanks for following up.

[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?

[dneilson]

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]

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

[reames]

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.

[dneilson]

• Addressing review comments.

[reames]

LGTM w/minor comment applied.

lib/IR/IRBuilder.cpp

126 ↗

(On Diff #129763)

very minor:
if (Align > 0)

cast<MemCpyInst>(CI)->setAlignment(Align);

[dneilson]

• Address the minor comment.
• Rebase.
• clang-format