[PATCH] D41675: Remove alignment argument from memcpy/memmove/memset in favour of alignment attributes (Step 1)

Hello,

Is there a script to update those test cases? I see mention of a sed script in the commit message but when I try it (see attached) on sed I get the following error:

sed: file script line 2: invalid reference \3 on `s’ command’s RHS

Did I lose something in a copy-paste? Is it not really a sed script? How do I run it?

memcopy.sed (3.65 KB)

Hi Alexandre,
The script uses extended-sed syntax, so you need to run sed with the -E option.

For example, when preparing the patch I created a file ( script.sed ) containing all of the lines that I copied into the commit message. Then, I ran this bash one-liner from the test directory:
for f in $(find . -name ‘*.ll’); do sed -E -i ‘.sedbak’ -f script.sed $f; done

When I was happy with the results, then: find . -name ‘*.sedbak’ --exec rm -f {} ;

Please let me know if that doesn’t work for you.

-Daniel

Thanks, that worked like a charm except for the following:

llvm generate:

call void @llvm.memcpy.p3i8.p1i8.i64(i8 addrspace(3)* align 1 bitcast ([512 x float] addrspace(3)* @a_scratchpad to i8 addrspace(3)), i8 addrspace(1) align 1 %0, i64 2048, i1 false)

And we expected:

call void @llvm.memcpy.p3i8.p1i8.i64(i8 addrspace(3)* bitcast ([512 x float] addrspace(3)* [[SPM0]] to i8 addrspace(3)), i8 addrspace(1) [[APTR]], i64 2048, i1 false)

Notice the presence of “align 1”. I’m not sure which side is correct, isn’t it equivalent (that is, this is the natural ABI alignment of that type)?

Here is my datalayout:

target datalayout = “e-m:e-i64:64-n8:16:32:64-S128-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024”

Hi Alexandre,
Before the change you would have been expecting one of the following, correct?
a) call void @llvm.memcpy.p3i8.p1i8.i64(i8 addrspace(3)* bitcast ([512 x float] addrspace(3)* [[SPM0]] to i8 addrspace(3)), i8 addrspace(1) [[APTR]], i64 2048, i32 0, i1 false)
b) call void @llvm.memcpy.p3i8.p1i8.i64(i8 addrspace(3)* bitcast ([512 x float] addrspace(3)* [[SPM0]] to i8 addrspace(3)), i8 addrspace(1) [[APTR]], i64 2048, i32 1, i1 false)

Functionally, (a) & (b) are both saying that the src & dest pointers are 1-byte aligned; i.e. they’re both basically saying “I don’t really have any better information on alignment, so we’ll go with 1-byte aligned since it can’t be less aligned than that”

After the patch, you’re seeing:
i) call void @llvm.memcpy.p3i8.p1i8.i64(i8 addrspace(3)* align 1 bitcast ([512 x float] addrspace(3)* [[SPM0]] to i8 addrspace(3)), i8 align 1 addrspace(1) [[APTR]], i64 2048, i1 false)
but your IR test, that you ran the sed script on, is saying to expect:
ii) call void @llvm.memcpy.p3i8.p1i8.i64(i8 addrspace(3)* bitcast ([512 x float] addrspace(3)* [[SPM0]] to i8 addrspace(3)), i8 addrspace(1) [[APTR]], i64 2048, i1 false)

Is that correct? Just like (a) & (b), both (i) & (ii) are functionally equivalent.

The script rule that would have changed that was:
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

It was converting both (a) and (b) into (ii). If that’s not what you’re seeing/wanting, then you can just add the ‘align 1’s into your test’s CHECK pattern, or alter the sed script by changing “…, i32 [01], i1…” into “…, i32 0, i1…"

-Daniel

Yes, all that is correct.

My question is more a long term question: why do the .ll printer specify the alignment if it is equivalent to the default one?
That is, it seems the sed script expect the printer to not specify it (this would match the load/store behavior), but the ll-printer does specify it, which either means the printer is not ideal on this case and I should fix it, or in this case the ABI alignment is not what I think it is, then I should fix the test-cases.

I’m not sure which side is correct.

Good question. AFAIK, the IR-printer doesn’t understand the semantics of parameter attributes. In this case, it only knows that there is an attribute on the parameter that is integer valued (with value 1) and that has the name “align”, so it prints it out. If we don’t want it printing out ‘align 1’ then it’s up to us to not set the alignment parameter attribute to a value if that value would be 1.

If preferred, it would be easy enough to change the behaviour for the memory intrinsics to only set the attribute to a value if an alignment greater than 1 is supplied. It’d keep the text in the IR a little cleaner/cleaner, and not change functionality…

Thoughts/opinions, anyone?

-Daniel

Ah, I see. That means I should just skip the setting of the alignment if it matches the ABI (this one is produced by a custom pass). Thanks.

Note that the default ABI alignment might be bigger than 1!

This seems like a reasonable proposal for any param alignment attribute. I don’t see a reason to restrict this to the memory intrinsics.