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

[Pete Cooper via llvm-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 <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...
> 
> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150928/ad2c65f2/attachment.html>