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

[Hal Finkel via cfe-commits]

----- Original Message -----

> From: "Pete Cooper" <peter_cooper at apple.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Lang Hames" <lhames at apple.com>, "LLVM Commits"
> <llvm-commits at lists.llvm.org>, cfe-commits at 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 < hfinkel at 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<bool> isDestVolatile = None, Optional<bool>
> > 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_cooper at apple.com >
> > 
> 
> > > To: "Hal J. Finkel" < hfinkel at anl.gov >
> > 
> 
> > > Cc: "Lang Hames" < lhames at apple.com >, "LLVM Commits" <
> > > llvm-commits at lists.llvm.org >, cfe-commits at 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
> 

-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151111/01203dc6/attachment-0001.html>