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

[Hal Finkel via llvm-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" <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