Page MenuHomePhabricator

Add ARM cdp intrinsics
AbandonedPublic

Authored by rengolin on May 17 2016, 6:01 AM.

Details

Reviewers
rs
Summary

Please review my patch to add the \_\_arm_cdp and \_\_arm_cdp2 intrinsics. These intrinsics don't exist in the latest ACLE (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ihi0053c/index.html) but will exist in a future version.

Diff Detail

Repository
rL LLVM

Event Timeline

rs updated this revision to Diff 57467.May 17 2016, 6:01 AM
rs retitled this revision from to Add ARM cdp intrinsics.
rs updated this object.
rs added a reviewer: rengolin.
rs set the repository for this revision to rL LLVM.
rs updated this object.May 17 2016, 6:02 AM
rs added a subscriber: cfe-commits.May 17 2016, 6:06 AM
rengolin requested changes to this revision.May 17 2016, 6:39 AM
rengolin edited edge metadata.

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

include/clang/Basic/BuiltinsARM.def
55

I wonder why the old signature was wrong... probably because the docs weren't public and no one could really check whether they were right or not. I don't want to repeat the same mistake.

test/CodeGen/arm-coproc-intrinsics.c
1

Please, do not add assembly tests in Clang. This test should be in LLVM.

This revision now requires changes to proceed.May 17 2016, 6:39 AM

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 specifications with no implementation yet is a bit annoying too.

We may discover bugs (though this seems fairly straightforward), but I think it's highly unlikely we'll have vocal and powerful enough users of such a niche feature to prevent us fixing them.

I agree about the asm tests though, they belong in LLVM.

include/clang/Basic/BuiltinsARM.def
55

The difference between "UIi" and "Ui" is that the fix requires a constant. A pretty obvious restriction even without a specification.

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 specifications with no implementation yet is a bit annoying too.

I don't think a small future ACLE builtin is on the same league as a whole new back-end. :)

Nor I think that having this when the docs are public, rather than now, will affect users in any way.

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 addition in the new ACLE, so why not release the document, and then submit all changes then?

Or, maybe I am mistaken, and this is really that important... is it?

cheers,
--renato

include/clang/Basic/BuiltinsARM.def
55

Ah, this makes sense. Looks like a good change in its own.

rs marked an inline comment as done.May 18 2016, 2:23 AM

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 addition in the new ACLE, so why not release the document, and then submit all changes then?
Or, maybe I am mistaken, and this is really that important... is it?

Thanks for reviewing.

It's not that important to have the intrinsic added before the document is released. I'll upload a new patch without the intrinsic in arm_acle.h and I'll move the assembly test to LLVM.

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

rs added a comment.May 18 2016, 3:56 PM

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

rs added a comment.May 18 2016, 3:58 PM

http://reviews.llvm.org/D20394 which adds a test for the intrinsic in llvm

Wrong link, should be http://reviews.llvm.org/D20393

rengolin commandeered this revision.Jun 27 2016, 7:09 AM
rengolin abandoned this revision.
rengolin edited reviewers, added: rs; removed: rengolin.