llvm dev - Jan 2018 - Change memcpy/memmove/memset to have dest and source alignment attributes

> On Jan 18, 2018, at 7:45 AM, Daniel Neilson via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> 
> Hi all,
>  This change has been reviewed, and appears to be ready to land (review
available here if anyone still wants to chime in:
https://reviews.llvm.org/D41675 <https://reviews.llvm.org/D41675> ). The
process that we’re going to use for landing this will take a few steps. To wit:
> 
> Step 1) Remove align argument, and add align attribute to pointer args.
What does this mean? Are you suggesting that alignment become part of
PointerType?  That would introduce lots of problems, because unaligned and
aligned pointers would require casts, which then interfere with optimizations.

-Chris


> Require that src & dest have the same alignment via verifier rule. 
Also update Clang & Polly tests to pattern-match the new form.
> 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.
> 
>  Unless there are objections, I would like to land step 1 tomorrow (Friday
Jan 19) morning. My hope is to be completing up to step 4 by the middle of next
week, and then steps 5 & 6 in the coming weeks.
> 
>  Landing step 1 will potentially have a large impact on downstream
consumer’s tests. For example, a front-end that checks IR generation of
constructs implemented using memcpy/memmove/memset; the Clang tests for OpenMP
firstprivate/lastprivate clauses, for instance, required a lot of manual
updating. These sorts of tests will need to be updated to match the new pattern
printed for the memory intrinsics. I’ve included an extended sed script in the
commit log that can update many patterns, but definitely does not catch them all
so some manual updating will be required.
> 
> -Daniel
> 
> ---
> Daniel Neilson, Ph.D.
> Azul Systems
> 
>> On Jan 2, 2018, at 2:11 PM, Daniel Neilson via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>> 
>> Good day all,
>>  I’ve spent a few days resurrecting the circa-2015 work on removing the
explicit alignment argument (4th arg) from the @llvm.memcpy/memmove/memset
intrinsics in favour of using the alignment attribute on the pointer args of
calls to the intrinsic. This work was first proposed back in August 2015 by Lang
Hames:
>> http://lists.llvm.org/pipermail/llvm-dev/2015-August/089384.html
<http://lists.llvm.org/pipermail/llvm-dev/2015-August/089384.html>   (item
2)
>>  and an attempt at landing the work was made by Pete Cooper in November
2015, but then backed out due to unspecified bot failures:
>>
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20151109/312083.html
<http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20151109/312083.html>
>> 
>>  I’ve prepared changes for LLVM, Clang, and Polly that are now up for
review:
>> * https://reviews.llvm.org/D41675
<https://reviews.llvm.org/D41675>   (LLVM part)
>> * https://reviews.llvm.org/D41676
<https://reviews.llvm.org/D41676>   (polly part)
>> * https://reviews.llvm.org/D41677
<https://reviews.llvm.org/D41677>   (Clang part)
>> 
>>  Importantly for those maintaining downstream users of the LLVM API,
this changes the prototypes for the @llvm.memcpy/memmove/memset intrinsics and
changes the IRBuilder API for creating memcpy and memmove calls.
>> 
>> For example, IR 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)
>> 
>>  The LLVM change includes auto upgrade of the old IR. However, match
expressions in IR tests and calls to IRBuilder’s CreateMemCpy &
CreateMemMove will need to be updated.
>> 
>>  My plan is to post another note to the list when the change is landed,
and stable.
>> 
>>  Comments? Concerns?
>> 
>> -Daniel
>> 
>> ---
>> Daniel Neilson, Ph.D.
>> Azul Systems
>> 
>> 
>> 
>> 
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180118/b4a9b1c1/attachment.html>

On Jan 18, 2018, at 10:48 PM, Chris Lattner <clattner at
nondot.org<mailto:clattner at nondot.org>> wrote:



On Jan 18, 2018, at 7:45 AM, Daniel Neilson via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:


Hi all,
 This change has been reviewed, and appears to be ready to land (review
available here if anyone still wants to chime in:
https://reviews.llvm.org/D41675 ). The process that we’re going to use for
landing this will take a few steps. To wit:

Step 1) Remove align argument, and add align attribute to pointer args.

What does this mean? Are you suggesting that alignment become part of
PointerType?  That would introduce lots of problems, because unaligned and
aligned pointers would require casts, which then interfere with optimizations.

Bad communication on my part; perhaps, I used non-standard terminology to refer
to a CallInst’s parameter attributes. I am not touching the definition of
PointerType in any way; I don’t see the need, and I’m not that ambitious. ;-)
Currently, a call to @llvm.memcpy might look like this:

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

where the ‘i32 4’ argument (2nd last arg) is the minimum alignment of both of
the pointer args (%src & %dest). After this change, this same call will
instead read:

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

where we use an align attribute on first two function-call arguments. (i.e
CallInst::addParamAttr(0, …) & CallInst::addParamAttr(1, …))

 That is all that this first step is doing. With the caveat that we, temporarily
(basically for the purposes of testing the initial change), will require that
the alignment attribute on both of the pointer args be the same value. So,
nothing like this for now:

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

 The alignments-equal restriction will be lifted in step 2, which should happen
next week.

 Is that clearer?

-Daniel

---
Daniel Neilson, Ph.D.
Azul Systems

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180119/fef79da7/attachment.html>

> On Jan 18, 2018, at 9:38 PM, Daniel Neilson <dneilson at azul.com>
wrote:
> 
> 
>> On Jan 18, 2018, at 10:48 PM, Chris Lattner <clattner at nondot.org
<mailto:clattner at nondot.org>> wrote:
>> 
>> 
>> 
>>> On Jan 18, 2018, at 7:45 AM, Daniel Neilson via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
>>> 
>>> 
>>> Hi all,
>>>  This change has been reviewed, and appears to be ready to land
(review available here if anyone still wants to chime in:
https://reviews.llvm.org/D41675 <https://reviews.llvm.org/D41675> ). The
process that we’re going to use for landing this will take a few steps. To wit:
>>> 
>>> Step 1) Remove align argument, and add align attribute to pointer
args.
>> 
>> What does this mean? Are you suggesting that alignment become part of
PointerType?  That would introduce lots of problems, because unaligned and
aligned pointers would require casts, which then interfere with optimizations.
> 
> Bad communication on my part; perhaps, I used non-standard terminology to
refer to a CallInst’s parameter attributes. I am not touching the definition of
PointerType in any way; I don’t see the need, and I’m not that ambitious. ;-)
Currently, a call to @llvm.memcpy might look like this:
> 
>   call void @llvm.memcpy.p0i8.p0i8.i32(i8* %dest, i8* %src, i32 100, i32 4,
i1 false)
> 
> where the ‘i32 4’ argument (2nd last arg) is the minimum alignment of both
of the pointer args (%src & %dest). After this change, this same call will
instead read:
> 
>   call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 %dest, i8* align 4 %src,
i32 100, i1 false)
> 
> where we use an align attribute on first two function-call arguments. (i.e
CallInst::addParamAttr(0, …) & CallInst::addParamAttr(1, …))
> 
>  That is all that this first step is doing. With the caveat that we,
temporarily (basically for the purposes of testing the initial change), will
require that the alignment attribute on both of the pointer args be the same
value. So, nothing like this for now:
> 
>   call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 16 %dest, i8* align 4
%src, i32 100, i1 false)
> 
>  The alignments-equal restriction will be lifted in step 2, which should
happen next week.
Awesome, this sounds like a great approach to me!  Thanks Daniel,

-Chris


-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180118/3977f843/attachment.html>