Re: [PATCH] Change memcpy/memmove/memset to have dest and source alignment

[Pete Cooper via cfe-commits]

Pushed this in LLVM r253511 and clang 253512.

Thanks again for the review Hal.

BTW, I realized the docs need to be updated.  I’ll let the bots run on this for 
a while and if it all goes well I’ll commit the docs update.

Cheers,
Pete
> On Nov 11, 2015, at 11:25 AM, Pete Cooper via cfe-commits 
>  wrote:
> 
> 
>> On Nov 11, 2015, at 11:16 AM, Hal Finkel > > wrote:
>> 
>> It seems like I dropped the ball on this. Yes, I recall being fine with them 
>> otherwise.
> Thanks Hal!  I’ll rebase the patches and land them over the next day or two.
> 
> Thanks,
> Pete
>> 
>> Thanks again,
>> Hal
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] Change memcpy/memmove/memset to have dest and source alignment

[Hal Finkel via cfe-commits]

- Original Message -

> From: "Pete Cooper" <peter_coo...@apple.com>
> To: "Hal Finkel" <hfin...@anl.gov>
> Cc: "Lang Hames" <lha...@apple.com>, "LLVM Commits"
> <llvm-comm...@lists.llvm.org>, cfe-commits@lists.llvm.org
> Sent: Monday, September 28, 2015 12:46:36 PM
> Subject: Re: [PATCH] Change memcpy/memmove/memset to have dest and
> source alignment

> Hey Hal

> Thanks for the review. I really appreciate it given the scale of
> this.

> > On Sep 25, 2015, at 1:13 PM, Hal Finkel < hfin...@anl.gov > wrote:
> 

> > Hi Pete,
> 

> > Thanks for working on this.
> 

> > + class IntegerAlignment {
> 
> > + private:
> 
> > + uint64_t Align;
> 

> > You explain in the patch summary why this is here, but please add a
> > comment with the explanation as well.
> 

> Will do. Good catch.

> > Regarding the auto-upgrade, are we going to run into problems if we
> > separate our the 'volatile' tag for the source and the destination
> > as Lang had suggested? If we're going to do that, should we do it
> > all at the same time? Does it change the need for the
> > IntegerAlignment class?
> 

> Luckily I think this will be easier as its just an addition, unlike
> this patch which is moving things around to attributes. I’m also
> pretty confident about using sed to auto-upgrade all the tests with
> the volatile flag. Its much easier to sed for the ‘i1 [true|false])’
> at the end of the call and just duplicate that piece, than it is to
> extract the alignment out from the middle and print it back out as
> an attribute.

> Saying that, there will be similar churn on things like the
> IRBuilder, and we may want some way to (temporarily at the least)
> make sure that everyone is forced to consider whether they want to
> set both isVolatile flags given that they set one of them. A few
> possibilities are:

> > // Bad, because we don’t know whether current users who set
> > isDestVolatile to true also want source volatility too
> 

> > Create(..., bool isDestVolatile = false, bool isSrcVolatile =
> > false)
> 

> > // Better, all current users will have to change anyway, and then
> > we
> > know they all care about src/dest volatility
> 

> > Create(…, std::pair<bool, bool> isDestSrcVolatile = std::pair<bool,
> > bool>(false, false))
> 

> > // Another option, and we would use an assert perhaps to make sure
> > that if dest is set, then so is src
> 

> > Create(…, Optional isDestVolatile = None, Optional
> > isSrcVolatile = None) {
> 

> > assert(isDestVolatile.hasValue() == isSrcVolatile.hasValue());
> 

> > …
> 
> Anyway, I think changes can come later, but just a few ideas there.

> > Everything else looks good, and I like the cleanup in
> > AlignmentFromAssumptions :-)
> 

> Thanks :) Can I take that as a LGTM, or...

It seems like I dropped the ball on this. Yes, I recall being fine with them 
otherwise. 

Thanks again, 
Hal 

> > Thanks again,
> 
> > Hal
> 

> > P.S. I find full-context patches in Phabricator much easier than
> > diffs; this does not matter for the automated regression-test
> > updates, but for the code changes, I appreciate patches there.
> 

> … would you prefer to see the patch in phab first? I’m happy either
> way. I will leave the test case changes here though, and just put
> the code in phab if needed and if thats ok.

> Cheers,
> Pete

> > - Original Message -
> 

> > > From: "Pete Cooper" < peter_coo...@apple.com >
> > 
> 
> > > To: "Hal J. Finkel" < hfin...@anl.gov >
> > 
> 
> > > Cc: "Lang Hames" < lha...@apple.com >, "LLVM Commits" <
> > > llvm-comm...@lists.llvm.org >, cfe-commits@lists.llvm.org
> > 
> 
> > > Sent: Friday, August 28, 2015 5:59:18 PM
> > 
> 
> > > Subject: [PATCH] Change memcpy/memmove/memset to have dest and
> > > source
> > > alignment
> > 
> 

> > > Hi Hal/Lang
> > 
> 

> > > This came out of a discussion here:
> > 
> 
> > > http://lists.llvm.org/pipermail/llvm-dev/2015-August/089384.html
> > 
> 

> > > We want to be able to provide alignment for both the source and
> > > dest
> > 
> 
> > > of mem intrinsics, instead of the alignment argument which only
> > 
> 
> > > provides the minimum of dest/src.
> > 
> 

> > > Instead of adding another argument, I removed the alignment
> > > argument
> > 
> 

Re: [PATCH] Change memcpy/memmove/memset to have dest and source alignment

[Pete Cooper via cfe-commits]

Hey Hal

Thanks for the review. I really appreciate it given the scale of this.
> On Sep 25, 2015, at 1:13 PM, Hal Finkel  wrote:
> 
> Hi Pete,
> 
> Thanks for working on this.
> 
> +  class IntegerAlignment {
> +  private:
> +uint64_t Align;
> 
> You explain in the patch summary why this is here, but please add a comment 
> with the explanation as well.
Will do.  Good catch.
> 
> Regarding the auto-upgrade, are we going to run into problems if we separate 
> our the 'volatile' tag for the source and the destination as Lang had 
> suggested? If we're going to do that, should we do it all at the same time? 
> Does it change the need for the IntegerAlignment class?
Luckily I think this will be easier as its just an addition, unlike this patch 
which is moving things around to attributes.  I’m also pretty confident about 
using sed to auto-upgrade all the tests with the volatile flag.  Its much 
easier to sed for the ‘i1 [true|false])’ at the end of the call and just 
duplicate that piece, than it is to extract the alignment out from the middle 
and print it back out as an attribute.

Saying that, there will be similar churn on things like the IRBuilder, and we 
may want some way to (temporarily at the least) make sure that everyone is 
forced to consider whether they want to set both isVolatile flags given that 
they set one of them.  A few possibilities are:

// Bad, because we don’t know whether current users who set isDestVolatile to 
true also want source volatility too
Create(..., bool isDestVolatile = false, bool isSrcVolatile = false) 

// Better, all current users will have to change anyway, and then we know they 
all care about src/dest volatility
Create(…, std::pair isDestSrcVolatile = std::pair(false, false))

// Another option, and we would use an assert perhaps to make sure that if dest 
is set, then so is src
Create(…, Optional isDestVolatile = None, Optional isSrcVolatile = 
None) {
  assert(isDestVolatile.hasValue() == isSrcVolatile.hasValue());
  …

Anyway, I think changes can come later, but just a few ideas there.
> 
> Everything else looks good, and I like the cleanup in 
> AlignmentFromAssumptions :-)
Thanks :)  Can I take that as a LGTM, or...
> 
> Thanks again,
> Hal
> 
> P.S. I find full-context patches in Phabricator much easier than diffs; this 
> does not matter for the automated regression-test updates, but for the code 
> changes, I appreciate patches there.
… would you prefer to see the patch in phab first?  I’m happy either way.  I 
will leave the test case changes here though, and just put the code in phab if 
needed and if thats ok.

Cheers,
Pete
> 
> - Original Message -
>> From: "Pete Cooper" 
>> To: "Hal J. Finkel" 
>> Cc: "Lang Hames" , "LLVM Commits" 
>> , cfe-commits@lists.llvm.org
>> Sent: Friday, August 28, 2015 5:59:18 PM
>> Subject: [PATCH] Change memcpy/memmove/memset to have dest and source 
>> alignment
>> 
>> 
>> 
>> Hi Hal/Lang
>> 
>> This came out of a discussion here:
>> http://lists.llvm.org/pipermail/llvm-dev/2015-August/089384.html
>> 
>> We want to be able to provide alignment for both the source and dest
>> of mem intrinsics, instead of the alignment argument which only
>> provides the minimum of dest/src.
>> 
>> Instead of adding another argument, I removed the alignment argument
>> and added the align attributes to the src/dest pointer arguments.
>> 
>> I’ve updated the MemIntrinsic definition to handle this, and all of
>> the code to now call getDestAlignment/getSrcAlignment instead of
>> getAlignment. For the few places where it wasn’t clear whether
>> dest/src was the right choice, i’ve left a FIXME and I take the
>> minimum of dest/src so as to match the current behavior.
>> 
>> I’ve also updated the create methods in the IR builder. There is a
>> (temporary) class there to handle the new source alignment
>> parameter, as otherwise existing callers of this code could end up
>> having the isVolatile bool implicitly converted to the source
>> alignment. I’ll remove this once out-of-tree users have had a chance
>> to update to this patch.
>> 
>> Tests were updated to automatically strip out the alignment argument
>> (the regex find/replace is in the patch). I then ran make check and
>> explicitly added back in alignments to all tests which needed it. I
>> tried to automatically update tests to transfer alignment to the
>> attributes, but that wasn’t feasible due to constant expressions
>> confusing regex (turns out regex doesn’t like recursion, which is
>> more or less what you need to get balanced braces in gep(bit
>> cast(inttoptr())) type expressions which we have in our tests).
>> 
>> There’s also a commit which shows how auto upgrading bitcode is
>> handled. There was already a test for llvm 3.2 which called memcpy
>> so is used for this. I’ll add tests to upgrade memmove and memset
>> prior to committing 

Re: [PATCH] Change memcpy/memmove/memset to have dest and source alignment

[Hal Finkel via cfe-commits]

Hi Pete,

Thanks for working on this.

+  class IntegerAlignment {
+  private:
+uint64_t Align;

You explain in the patch summary why this is here, but please add a comment 
with the explanation as well.

Regarding the auto-upgrade, are we going to run into problems if we separate 
our the 'volatile' tag for the source and the destination as Lang had 
suggested? If we're going to do that, should we do it all at the same time? 
Does it change the need for the IntegerAlignment class?

Everything else looks good, and I like the cleanup in AlignmentFromAssumptions 
:-)

Thanks again,
Hal

P.S. I find full-context patches in Phabricator much easier than diffs; this 
does not matter for the automated regression-test updates, but for the code 
changes, I appreciate patches there.

- Original Message -
> From: "Pete Cooper" 
> To: "Hal J. Finkel" 
> Cc: "Lang Hames" , "LLVM Commits" 
> , cfe-commits@lists.llvm.org
> Sent: Friday, August 28, 2015 5:59:18 PM
> Subject: [PATCH] Change memcpy/memmove/memset to have dest and source 
> alignment
> 
> 
> 
> Hi Hal/Lang
> 
> This came out of a discussion here:
> http://lists.llvm.org/pipermail/llvm-dev/2015-August/089384.html
> 
> We want to be able to provide alignment for both the source and dest
> of mem intrinsics, instead of the alignment argument which only
> provides the minimum of dest/src.
> 
> Instead of adding another argument, I removed the alignment argument
> and added the align attributes to the src/dest pointer arguments.
> 
> I’ve updated the MemIntrinsic definition to handle this, and all of
> the code to now call getDestAlignment/getSrcAlignment instead of
> getAlignment. For the few places where it wasn’t clear whether
> dest/src was the right choice, i’ve left a FIXME and I take the
> minimum of dest/src so as to match the current behavior.
> 
> I’ve also updated the create methods in the IR builder. There is a
> (temporary) class there to handle the new source alignment
> parameter, as otherwise existing callers of this code could end up
> having the isVolatile bool implicitly converted to the source
> alignment. I’ll remove this once out-of-tree users have had a chance
> to update to this patch.
> 
> Tests were updated to automatically strip out the alignment argument
> (the regex find/replace is in the patch). I then ran make check and
> explicitly added back in alignments to all tests which needed it. I
> tried to automatically update tests to transfer alignment to the
> attributes, but that wasn’t feasible due to constant expressions
> confusing regex (turns out regex doesn’t like recursion, which is
> more or less what you need to get balanced braces in gep(bit
> cast(inttoptr())) type expressions which we have in our tests).
> 
> There’s also a commit which shows how auto upgrading bitcode is
> handled. There was already a test for llvm 3.2 which called memcpy
> so is used for this. I’ll add tests to upgrade memmove and memset
> prior to committing but just wanted to get the review process
> started. memmove/memset tests will be almost identical to the memcpy
> test we already have. I will use 3.7 as the bitcode generator for
> those tests though.
> 
> Finally, for LLVM, i’ve got a patch to show a cleanup of
> AlignmentFromAssumptions now that dest and source alignments can be
> different. This is the only patch which can be committed on its own,
> and after the rest. Everything else here is broken out in to
> separate commits just to try ease the review process.
> 
> For clang, the majority of changes are to pass both source and dest
> alignments to the IR Builder. Again I tried to get the right ones
> where I could, but if I didn’t know the code well enough I just pass
> the same value to dest/src to match the current behaviour. clang
> tests were all updated by hand because they all needed to check for
> the alignment attribute being set correctly coming out of codegen.
> 
> Finally, this is really just the minimum changes needed to get this
> in tree. I haven’t made any effort to teach LLVM codegen about this.
> That can luckily be done later, and probably independently for most
> backends by their respective owners.
> 
> Apologies for the scale of this patch. As you can imagine, there
> wasn’t really a nice way to land this without excessive churn of the
> test cases themselves.
> 
> Cheers,
> Pete
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits