----- 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<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_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 > > > > > > 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
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits