Re: [PATCH] D20325: Add ARM cdp intrinsics

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

Re: [PATCH] D20325: Add ARM cdp intrinsics

[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

http://reviews.llvm.org/D20325



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20325: Add ARM cdp intrinsics

[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 to existing documentation, because the documentation is
still in draft. It's either that or wait until the documentation is ready,
which can be quite some time (there is no v8.1A ARMARM yet...)

The ACLE in particular stays in draft for some time, because it often waits
for implementations to be done in GCC and LLVM to see if the specification
is actually workable!

Cheers,

James

On Wed, 18 May 2016 at 11:03 Renato Golin via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> 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
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D20325
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20325: Add ARM cdp intrinsics

[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


Repository:
  rL LLVM

http://reviews.llvm.org/D20325



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20325: Add ARM cdp intrinsics

[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 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.


Repository:
  rL LLVM

http://reviews.llvm.org/D20325



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20325: Add ARM cdp intrinsics

[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 proper already, and I don't think
> even the v8.1 specification is public (all I can find is a blog
> announcing it).

That's a good point...

It shouldn't hurt to add this one, I agree, I just don't want us
changing again when the spec comes out because of some silly typo.

I mean, even that is irrelevant, but I don't like to set the precedent
where we start implementing random things and changing our minds every
other month.

Still, for this patch to go in, it'll need some more changes... I'll
comment on the patch, since it seems Phab didn't pick up your reply...
:(

--renato
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20325: Add ARM cdp intrinsics

[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 docs are public, rather than now, will 
> affect users in any way.

True. It's an extremely niche feature.

> 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.

The only place I've seen that policy in play is with asm aliases
(which have a habit of getting documented nowhere) and even there we
generally grudgingly accept promises to document.

Generally we're far more relaxed as long as a specification is on the
way. We're up to to v8.2 in LLVM proper already, and I don't think
even the v8.1 specification is public (all I can find is a blog
announcing it).

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

Goodness no! The entire coprocessor instruction space was
deprecated/reclaimed in v8 as far as I'm aware. But since I don't
think timing matters particularly either way and ARM themselves are
the ones who will be supporting the majority of these users, it's
pretty harmless for us to fall in with their desired schedules.



Well, that's how I view these kinds of things anyway.

Tim.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20325: Add ARM cdp intrinsics

[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", "")
+BUILTIN(__builtin_arm_cdp, "vUIiUIiUIiUIiUIiUIi", "")
+BUILTIN(__builtin_arm_cdp2, "vUIiUIiUIiUIiUIiUIi", "")

t.p.northover wrote:
> rengolin wrote:
> > 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.
> The difference between "UIi" and "Ui" is that the fix requires a constant. A 
> pretty obvious restriction even without a specification.
Ah, this makes sense. Looks like a good change in its own.


Repository:
  rL LLVM

http://reviews.llvm.org/D20325



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20325: Add ARM cdp intrinsics

[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 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


Repository:
  rL LLVM

http://reviews.llvm.org/D20325



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20325: Add ARM cdp intrinsics

[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 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.



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", "")
+BUILTIN(__builtin_arm_cdp, "vUIiUIiUIiUIiUIiUIi", "")
+BUILTIN(__builtin_arm_cdp2, "vUIiUIiUIiUIiUIiUIi", "")

rengolin wrote:
> 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.
The difference between "UIi" and "Ui" is that the fix requires a constant. A 
pretty obvious restriction even without a specification.


Repository:
  rL LLVM

http://reviews.llvm.org/D20325



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20325: Add ARM cdp intrinsics

[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



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", "")
+BUILTIN(__builtin_arm_cdp, "vUIiUIiUIiUIiUIiUIi", "")
+BUILTIN(__builtin_arm_cdp2, "vUIiUIiUIiUIiUIiUIi", "")

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.


Comment at: test/CodeGen/arm-coproc-intrinsics.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 -Wall -triple thumbv7-eabi -target-cpu cortex-a8 -S -o - %s 
| FileCheck %s
+

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


Repository:
  rL LLVM

http://reviews.llvm.org/D20325



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits