Re: [PATCH] D20325: Add ARM cdp intrinsics

2016-05-18 Thread Ranjeet Singh via cfe-commits
rs added a comment. > http://reviews.llvm.org/D20394 which adds a test for the intrinsic in llvm Wrong link, should be http://reviews.llvm.org/D20393 Repository: rL LLVM http://reviews.llvm.org/D20325 ___ cfe-commits mailing list

Re: [PATCH] D20325: Add ARM cdp intrinsics

2016-05-18 Thread Ranjeet Singh via cfe-commits
rs added a comment. Hi Renato, I've created 2 new reviews for this work http://reviews.llvm.org/D20394 which adds a test for the intrinsic in llvm and http://reviews.llvm.org/D20394 which fixes the builtin signature for the cdp intrinsic. Thanks, Ranjeet Repository: rL LLVM

Re: [PATCH] D20325: Add ARM cdp intrinsics

2016-05-18 Thread James Molloy via cfe-commits
Hi, To add my oar in, I agree with Tim here. It is regrettable but true that documentation, be that the ARMARM or ACLE tends to lag behind our development. If LLVM wants to be at the leading edge of architecture support (I hope it does!) then patches will just have to be accepted without pointers

Re: [PATCH] D20325: Add ARM cdp intrinsics

2016-05-18 Thread Renato Golin via cfe-commits
rengolin added a comment. Thanks Ranjeet, The tests don't really need the new builtin to exist at all and can be added now. When you submit the __arm_cdp patch, you just need to make sure Clang generates a call to @llvm.arm.cdp and everything else will be covered. cheers, --renato

Re: [PATCH] D20325: Add ARM cdp intrinsics

2016-05-18 Thread Ranjeet Singh via cfe-commits
rs marked an inline comment as done. rs added a comment. > It's been our stance for a long time to require docs to approve changes, > however small. I don't want to relax that which I think is a good constraint, > not for such a seemly irrelevant issue. > I also doubt this will be the only

Re: [PATCH] D20325: Add ARM cdp intrinsics

2016-05-18 Thread Renato Golin via cfe-commits
On 18 May 2016 at 03:45, Tim Northover via cfe-commits wrote: > Well yes, it's probably got orders of magnitude less bugs than the > backend for a start. :D > Generally we're far more relaxed as long as a specification is on the > way. We're up to to v8.2 in LLVM

Re: [PATCH] D20325: Add ARM cdp intrinsics

2016-05-17 Thread Tim Northover via cfe-commits
On 17 May 2016 at 10:32, Renato Golin wrote: > I don't think a small future ACLE builtin is on the same league as a whole > new back-end. :) Well yes, it's probably got orders of magnitude less bugs than the backend for a start. > Nor I think that having this when the

Re: [PATCH] D20325: Add ARM cdp intrinsics

2016-05-17 Thread Renato Golin via cfe-commits
rengolin added inline comments. Comment at: include/clang/Basic/BuiltinsARM.def:55 @@ -54,3 +54,3 @@ BUILTIN(__builtin_arm_mrc2, "UiUIiUIiUIiUIiUIi", "") -BUILTIN(__builtin_arm_cdp, "vUiUiUiUiUiUi", "") -BUILTIN(__builtin_arm_cdp2, "vUiUiUiUiUiUi", "")

Re: [PATCH] D20325: Add ARM cdp intrinsics

2016-05-17 Thread Renato Golin via cfe-commits
rengolin added a comment. In http://reviews.llvm.org/D20325#432086, @t.p.northover wrote: > I don't think we need to wait until the document is public, necessarily. The > entire AArch64 backend went in before the encodings were public outside ARM > (except maybe in binutils source?) and

Re: [PATCH] D20325: Add ARM cdp intrinsics

2016-05-17 Thread Tim Northover via cfe-commits
t.p.northover added a subscriber: t.p.northover. t.p.northover added a comment. I don't think we need to wait until the document is public, necessarily. The entire AArch64 backend went in before the encodings were public outside ARM (except maybe in binutils source?) and releasing

Re: [PATCH] D20325: Add ARM cdp intrinsics

2016-05-17 Thread Renato Golin via cfe-commits
rengolin requested changes to this revision. rengolin added a comment. This revision now requires changes to proceed. Please, re-submit this change once the documents have been made public. Feel free to submit the assembly test to LLVM, using llc, on their own. cheers, --renato