llvm dev - Feb 2016 - [PATCH] D17497: Support arbitrary address space for intrinsics

This probably needs broader discussion.  We have an existing naming 
mechanism for polymorphic intrinsics; Elena is proposing a new one to 
avoid making the names for various load/store intrinsics particularly ugly.

My personal take:
1) I like the cleaner naming scheme.
2) I'm not sure the additional complexity is worth it.  (Not specific to 
the particular implementation proposed here.)
3) I have no strong preference other than that the @llvm.masked_load 
(and friends) intrinsics support alternate address spaces in some form 
in the near future.

What do others think?

Philip


-------- Forwarded Message --------
Subject: 	[PATCH] D17497: Support arbitrary address space for intrinsics
Date: 	Mon, 22 Feb 2016 08:39:38 +0000
From: 	Elena Demikhovsky <elena.demikhovsky at intel.com>
Reply-To: 	reviews+D17497+public+90f3d1b9468ba8ca at reviews.llvm.org
To: 	elena.demikhovsky at intel.com, apilipenko at azulsystems.com, 
listmail at philipreames.com, ayal.zaks at intel.com, 
Matthew.Arsenault at amd.com, pjcoup at gmail.com
CC: 	llvm-commits at lists.llvm.org



delena created this revision.
delena added reviewers: apilipenko, reames, Ayal, arsenm, pjcoup.
delena added a subscriber: llvm-commits.
delena set the repository for this revision to rL LLVM.

This is an alternative proposal for supporting address space in intrinsics.
It's applicable for any intrinsic, not only masked-load-store.
Related to http://reviews.llvm.org/D17270

I this proposal I add non-zero address space suffix to intrinsic name. The
original name looks like
@llvm.xxx.xxx
The same name with address space 1 :
@llvm.xxx.xxx.a_1

(I did not update documentation. I'll do this if the proposed change looks
reasonable for reviewers ).

Repository:
   rL LLVM

http://reviews.llvm.org/D17497

Files:
   ../include/llvm/IR/IRBuilder.h
   ../include/llvm/IR/Intrinsics.h
   ../lib/IR/Function.cpp
   ../lib/IR/IRBuilder.cpp
   ../lib/IR/Verifier.cpp
   ../test/Transforms/LoopVectorize/X86/gather_scatter.ll




-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160224/12aaed3a/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D17497.48653.patch
Type: text/x-patch
Size: 13401 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160224/12aaed3a/attachment-0001.bin>

My gut feeling is that it’s not worth it.  When we move from typed to untyped
pointers, we’re going to change the mangling from something like p200i8 to just
p200, which is already quite a bit cleaner, and actually looks cleaner to me
than the version proposed in this patch.

David
> On 24 Feb 2016, at 17:28, Philip Reames via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> This probably needs broader discussion.  We have an existing naming
mechanism for polymorphic intrinsics; Elena is proposing a new one to avoid
making the names for various load/store intrinsics particularly ugly.
> 
> My personal take:
> 1) I like the cleaner naming scheme.
> 2) I'm not sure the additional complexity is worth it.  (Not specific
to the particular implementation proposed here.)
> 3) I have no strong preference other than that the @llvm.masked_load (and
friends) intrinsics support alternate address spaces in some form in the near
future.
> 
> What do others think?
> 
> Philip
> 
> 
> -------- Forwarded Message --------
> Subject:	[PATCH] D17497: Support arbitrary address space for intrinsics
> Date:	Mon, 22 Feb 2016 08:39:38 +0000
> From:	Elena Demikhovsky <elena.demikhovsky at intel.com>
> Reply-To:	reviews+D17497+public+90f3d1b9468ba8ca at reviews.llvm.org
> To:	elena.demikhovsky at intel.com, apilipenko at azulsystems.com, listmail
at philipreames.com, ayal.zaks at intel.com, Matthew.Arsenault at amd.com,
pjcoup at gmail.com
> CC:	llvm-commits at lists.llvm.org
> 
> delena created this revision.
> delena added reviewers: apilipenko, reames, Ayal, arsenm, pjcoup.
> delena added a subscriber: llvm-commits.
> delena set the repository for this revision to rL LLVM.
> 
> This is an alternative proposal for supporting address space in intrinsics.
It's applicable for any intrinsic, not only masked-load-store.
> Related to 
> http://reviews.llvm.org/D17270
> 
> 
> I this proposal I add non-zero address space suffix to intrinsic name. The
original name looks like
> @llvm.xxx.xxx
> The same name with address space 1 :
> @llvm.xxx.xxx.a_1
> 
> (I did not update documentation. I'll do this if the proposed change
looks reasonable for reviewers ).
> 
> Repository:
>   rL LLVM
> 
> 
> http://reviews.llvm.org/D17497
> 
> 
> Files:
>   ../include/llvm/IR/IRBuilder.h
>   ../include/llvm/IR/Intrinsics.h
>   ../lib/IR/Function.cpp
>   ../lib/IR/IRBuilder.cpp
>   ../lib/IR/Verifier.cpp
>   ../test/Transforms/LoopVectorize/X86/gather_scatter.ll
> 
> 
> 
> 
> 
> <D17497.48653.patch>_______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

> On Feb 24, 2016, at 09:32, David Chisnall via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> My gut feeling is that it’s not worth it.  When we move from typed to
untyped pointers, we’re going to change the mangling from something like p200i8
to just p200, which is already quite a bit cleaner, and actually looks cleaner
to me than the version proposed in this patch.
> 
> David
+1 

The prettiness of intrinsic mangling is irrelevant, especially since this will
all change with typeless pointers anyway.

-Matt

One more possible solution that I considered is allowing forward reference in
types.

def int_xxx_yyy : Intrinsic<[LLVMVectorElt<1>], [llvm_anyvector_ty], 
[IntrNoMem]>;

For example, you want to add a vector operation that returns scalar:
In the current implementation it will look like
 
i32 @llvm.vec.i32.v8i32(<8 x i32>%x)

If you allow forward reference, it will look like

i32 @llvm.vec.v8i32(<8 x i32>%x).

The same about masked intrinsics. @llvm.masked.load.p0v8i32.v8i32 - you have a
long name with duplicated info just due to the limitation in the current
infrastructure.

-  Elena


-----Original Message-----
From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of David
Chisnall via llvm-dev
Sent: Wednesday, February 24, 2016 19:32
To: Philip Reames <listmail at philipreames.com>
Cc: llvm-dev <llvm-dev at lists.llvm.org>
Subject: Re: [llvm-dev] Fwd: [PATCH] D17497: Support arbitrary address space for
intrinsics

My gut feeling is that it’s not worth it.  When we move from typed to untyped
pointers, we’re going to change the mangling from something like p200i8 to just
p200, which is already quite a bit cleaner, and actually looks cleaner to me
than the version proposed in this patch.

David
> On 24 Feb 2016, at 17:28, Philip Reames via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> This probably needs broader discussion.  We have an existing naming
mechanism for polymorphic intrinsics; Elena is proposing a new one to avoid
making the names for various load/store intrinsics particularly ugly.
> 
> My personal take:
> 1) I like the cleaner naming scheme.
> 2) I'm not sure the additional complexity is worth it.  (Not specific 
> to the particular implementation proposed here.)
> 3) I have no strong preference other than that the @llvm.masked_load (and
friends) intrinsics support alternate address spaces in some form in the near
future.
> 
> What do others think?
> 
> Philip
> 
> 
> -------- Forwarded Message --------
> Subject:	[PATCH] D17497: Support arbitrary address space for intrinsics
> Date:	Mon, 22 Feb 2016 08:39:38 +0000
> From:	Elena Demikhovsky <elena.demikhovsky at intel.com>
> Reply-To:	reviews+D17497+public+90f3d1b9468ba8ca at reviews.llvm.org
> To:	elena.demikhovsky at intel.com, apilipenko at azulsystems.com, listmail
at philipreames.com, ayal.zaks at intel.com, Matthew.Arsenault at amd.com,
pjcoup at gmail.com
> CC:	llvm-commits at lists.llvm.org
> 
> delena created this revision.
> delena added reviewers: apilipenko, reames, Ayal, arsenm, pjcoup.
> delena added a subscriber: llvm-commits.
> delena set the repository for this revision to rL LLVM.
> 
> This is an alternative proposal for supporting address space in intrinsics.
It's applicable for any intrinsic, not only masked-load-store.
> Related to
> http://reviews.llvm.org/D17270
> 
> 
> I this proposal I add non-zero address space suffix to intrinsic name. 
> The original name looks like @llvm.xxx.xxx The same name with address 
> space 1 :
> @llvm.xxx.xxx.a_1
> 
> (I did not update documentation. I'll do this if the proposed change
looks reasonable for reviewers ).
> 
> Repository:
>   rL LLVM
> 
> 
> http://reviews.llvm.org/D17497
> 
> 
> Files:
>   ../include/llvm/IR/IRBuilder.h
>   ../include/llvm/IR/Intrinsics.h
>   ../lib/IR/Function.cpp
>   ../lib/IR/IRBuilder.cpp
>   ../lib/IR/Verifier.cpp
>   ../test/Transforms/LoopVectorize/X86/gather_scatter.ll
> 
> 
> 
> 
> 
> <D17497.48653.patch>_______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Elena,

I'd like to propose that we move forward withArtur's original patch 
<http://reviews.llvm.org/D17270> and separate the discussion of how we 
might change our intrinsic naming scheme.  Artur's patch is addressing a 
correctness problem; that has to overrule stylistic concerns.  We are 
seeing failures in our nightly tests due to this issue on an ongoing 
basis, and I'd really like to get the correctness issue resolved in the 
immediate future.

I am more than happy to continue the discussion about better naming 
schemes - in particular, I like you're idea of potentially allowing 
forward references - but I strongly feel we need to decouple it from a 
bug fix for a correctness issue.

Philip

On 02/24/2016 09:28 AM, Philip Reames via llvm-dev
wrote:
> This probably needs broader discussion.  We have an existing naming 
> mechanism for polymorphic intrinsics; Elena is proposing a new one to 
> avoid making the names for various load/store intrinsics particularly 
> ugly.
>
> My personal take:
> 1) I like the cleaner naming scheme.
> 2) I'm not sure the additional complexity is worth it.  (Not specific 
> to the particular implementation proposed here.)
> 3) I have no strong preference other than that the @llvm.masked_load 
> (and friends) intrinsics support alternate address spaces in some form 
> in the near future.
>
> What do others think?
>
> Philip
>
>
> -------- Forwarded Message --------
> Subject: 	[PATCH] D17497: Support arbitrary address space for intrinsics
> Date: 	Mon, 22 Feb 2016 08:39:38 +0000
> From: 	Elena Demikhovsky <elena.demikhovsky at intel.com>
> Reply-To: 	reviews+D17497+public+90f3d1b9468ba8ca at reviews.llvm.org
> To: 	elena.demikhovsky at intel.com, apilipenko at azulsystems.com, 
> listmail at philipreames.com, ayal.zaks at intel.com, 
> Matthew.Arsenault at amd.com, pjcoup at gmail.com
> CC: 	llvm-commits at lists.llvm.org
>
>
>
> delena created this revision.
> delena added reviewers: apilipenko, reames, Ayal, arsenm, pjcoup.
> delena added a subscriber: llvm-commits.
> delena set the repository for this revision to rL LLVM.
>
> This is an alternative proposal for supporting address space in intrinsics.
It's applicable for any intrinsic, not only masked-load-store.
> Related tohttp://reviews.llvm.org/D17270
>
> I this proposal I add non-zero address space suffix to intrinsic name. The
original name looks like
> @llvm.xxx.xxx
> The same name with address space 1 :
> @llvm.xxx.xxx.a_1
>
> (I did not update documentation. I'll do this if the proposed change
looks reasonable for reviewers ).
>
> Repository:
>    rL LLVM
>
> http://reviews.llvm.org/D17497
>
> Files:
>    ../include/llvm/IR/IRBuilder.h
>    ../include/llvm/IR/Intrinsics.h
>    ../lib/IR/Function.cpp
>    ../lib/IR/IRBuilder.cpp
>    ../lib/IR/Verifier.cpp
>    ../test/Transforms/LoopVectorize/X86/gather_scatter.ll
>
>
>
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160302/0b29ef98/attachment-0001.html>

Per my previous email, I have just signed off on Artur's original patch.

Philip

On 03/02/2016 11:21 AM, Philip Reames via llvm-dev
wrote:
> Elena,
>
> I'd like to propose that we move forward withArtur's original patch
> <http://reviews.llvm.org/D17270> and separate the discussion of how
we
> might change our intrinsic naming scheme.  Artur's patch is addressing 
> a correctness problem; that has to overrule stylistic concerns.  We 
> are seeing failures in our nightly tests due to this issue on an 
> ongoing basis, and I'd really like to get the correctness issue 
> resolved in the immediate future.
>
> I am more than happy to continue the discussion about better naming 
> schemes - in particular, I like you're idea of potentially allowing 
> forward references - but I strongly feel we need to decouple it from a 
> bug fix for a correctness issue.
>
> Philip
>
> On 02/24/2016 09:28 AM, Philip Reames via llvm-dev wrote:
>> This probably needs broader discussion.  We have an existing naming 
>> mechanism for polymorphic intrinsics; Elena is proposing a new one to 
>> avoid making the names for various load/store intrinsics particularly 
>> ugly.
>>
>> My personal take:
>> 1) I like the cleaner naming scheme.
>> 2) I'm not sure the additional complexity is worth it.  (Not
specific
>> to the particular implementation proposed here.)
>> 3) I have no strong preference other than that the @llvm.masked_load 
>> (and friends) intrinsics support alternate address spaces in some 
>> form in the near future.
>>
>> What do others think?
>>
>> Philip
>>
>>
>> -------- Forwarded Message --------
>> Subject: 	[PATCH] D17497: Support arbitrary address space for
intrinsics
>> Date: 	Mon, 22 Feb 2016 08:39:38 +0000
>> From: 	Elena Demikhovsky <elena.demikhovsky at intel.com>
>> Reply-To: 	reviews+D17497+public+90f3d1b9468ba8ca at reviews.llvm.org
>> To: 	elena.demikhovsky at intel.com, apilipenko at azulsystems.com, 
>> listmail at philipreames.com, ayal.zaks at intel.com, 
>> Matthew.Arsenault at amd.com, pjcoup at gmail.com
>> CC: 	llvm-commits at lists.llvm.org
>>
>>
>>
>> delena created this revision.
>> delena added reviewers: apilipenko, reames, Ayal, arsenm, pjcoup.
>> delena added a subscriber: llvm-commits.
>> delena set the repository for this revision to rL LLVM.
>>
>> This is an alternative proposal for supporting address space in
intrinsics. It's applicable for any intrinsic, not only masked-load-store.
>> Related tohttp://reviews.llvm.org/D17270
>>
>> I this proposal I add non-zero address space suffix to intrinsic name.
The original name looks like
>> @llvm.xxx.xxx
>> The same name with address space 1 :
>> @llvm.xxx.xxx.a_1
>>
>> (I did not update documentation. I'll do this if the proposed
change looks reasonable for reviewers ).
>>
>> Repository:
>>    rL LLVM
>>
>> http://reviews.llvm.org/D17497
>>
>> Files:
>>    ../include/llvm/IR/IRBuilder.h
>>    ../include/llvm/IR/Intrinsics.h
>>    ../lib/IR/Function.cpp
>>    ../lib/IR/IRBuilder.cpp
>>    ../lib/IR/Verifier.cpp
>>    ../test/Transforms/LoopVectorize/X86/gather_scatter.ll
>>
>>
>>
>>
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160304/5ec5be2a/attachment.html>