[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Alexander Batashev via Phabricator via cfe-commits]

alexbatashev marked 6 inline comments as done.
alexbatashev added a comment.

@Ka-Ka done


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62580/new/

https://reviews.llvm.org/D62580



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka accepted this revision.
Ka-Ka added a comment.

Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62580/new/

https://reviews.llvm.org/D62580



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added inline comments.



Comment at: clang/test/CodeGen/builtins-x86.c:127-128
 
-  tmp_V2LLi = __builtin_ia32_undef128();
-  tmp_V4LLi = __builtin_ia32_undef256();
+  tmp_V2LLi = (V2LLi)__builtin_ia32_undef128();
+  tmp_V4LLi = (V4LLi)__builtin_ia32_undef256();
 

I don't like the introduced casts. Can we change the testcase to operate on the 
appropriate types instead?
The above two lines could probably be replaced by:
  tmp_V2d = __builtin_ia32_undef128();
  tmp_V4d = __builtin_ia32_undef256();
What do you think?




Comment at: clang/test/CodeGen/builtins-x86.c:228-231
+  tmp_V8s = (V8s)__builtin_ia32_packsswb128(tmp_V8s, tmp_V8s);
+  tmp_V4i = (V4i)__builtin_ia32_packssdw128(tmp_V4i, tmp_V4i);
+  tmp_V8s = (V8s)__builtin_ia32_packuswb128(tmp_V8s, tmp_V8s);
   tmp_V8s = __builtin_ia32_pmulhuw128(tmp_V8s, tmp_V8s);

same here?



Comment at: clang/test/CodeGen/builtins-x86.c:250
   tmp_V4s = __builtin_ia32_phsubsw(tmp_V4s, tmp_V4s);
-  tmp_V16c = __builtin_ia32_pmaddubsw128(tmp_V16c, tmp_V16c);
+  tmp_V16c = (V16c)__builtin_ia32_pmaddubsw128(tmp_V16c, tmp_V16c);
   tmp_V8c = __builtin_ia32_pmaddubsw(tmp_V8c, tmp_V8c);

and here?



Comment at: clang/test/CodeGen/builtins-x86.c:428
   tmp_V4i = __builtin_ia32_psradi128(tmp_V4i, tmp_i);
-  tmp_V8s = __builtin_ia32_pmaddwd128(tmp_V8s, tmp_V8s);
+  tmp_V8s = (V8s)__builtin_ia32_pmaddwd128(tmp_V8s, tmp_V8s);
   (void) __builtin_ia32_monitor(tmp_vp, tmp_Ui, tmp_Ui);

Can we change this line to?
  tmp_V4i = __builtin_ia32_pmaddwd128(tmp_V8s, tmp_V8s);




Comment at: clang/test/CodeGen/builtins-x86.c:432
   tmp_V16c = __builtin_ia32_lddqu(tmp_cCp);
-  tmp_V2LLi = __builtin_ia32_palignr128(tmp_V2LLi, tmp_V2LLi, imm_i);
-  tmp_V1LLi = __builtin_ia32_palignr(tmp_V1LLi, tmp_V1LLi, imm_i);
+  tmp_V2LLi = (V2LLi)__builtin_ia32_palignr128((V16c)tmp_V2LLi, 
(V16c)tmp_V2LLi, imm_i);
+  tmp_V1LLi = (V1LLi)__builtin_ia32_palignr((V8c)tmp_V1LLi, (V8c)tmp_V1LLi, 
imm_i);

Can we change this line to?
  tmp_V16c = __builtin_ia32_palignr128(tmp_V16c, tmp_V16c, imm_i);




Comment at: clang/test/CodeGen/builtins-x86.c:433
+  tmp_V2LLi = (V2LLi)__builtin_ia32_palignr128((V16c)tmp_V2LLi, 
(V16c)tmp_V2LLi, imm_i);
+  tmp_V1LLi = (V1LLi)__builtin_ia32_palignr((V8c)tmp_V1LLi, (V8c)tmp_V1LLi, 
imm_i);
 #ifdef USE_SSE4

Can we change this line to?
  tmp_V8c = __builtin_ia32_palignr(tmp_V8c, tmp_V8c, imm_i);



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62580/new/

https://reviews.llvm.org/D62580



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.

LGTM! Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62580/new/

https://reviews.llvm.org/D62580



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Alexander Batashev via Phabricator via cfe-commits]

alexbatashev marked an inline comment as done.
alexbatashev added a comment.

@Ka-Ka good point. Thank you.
@Anastasia Would such tests be ok with you?
@erichkeane Thank you very much. I think I don't have permissions to commit 
changes and will need someone's help.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62580/new/

https://reviews.llvm.org/D62580



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

Do you think it's possible to add a test?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62580/new/

https://reviews.llvm.org/D62580



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added a comment.

What about a testcase? It shouldn't be hard to add a small testcase that 
demonstrate that the changed builtins now work when compiling OpenCL C code for 
x86. I don't think you have to add all changed builtins to the testcase (but a 
few to demonstrate the change).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62580/new/

https://reviews.llvm.org/D62580



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

Conditional approval, commit with the comment fixed.  Let me know if you need 
me to commit it for you.




Comment at: clang/include/clang/Basic/Builtins.def:56
 //  N   -> 'int' size if target is LP64, 'L' otherwise.
+//  O   -> long for OpenCL targets, long long otherwise
 //  S   -> signed

End the comment with a period.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62580/new/

https://reviews.llvm.org/D62580



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Alexander Batashev via Phabricator via cfe-commits]

alexbatashev added a comment.

In D62580#1521234 , @erichkeane wrote:

> 'O' is an interesting choice.  Any real justification for it, or just "what 
> was available"?  It definitely needs to be documented in the top of 
> Builtins.def however.


O is for **O**penCL. I planned to use 'C' but it is already used.

I'll add docs. Thank you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62580/new/

https://reviews.llvm.org/D62580



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

In D62580#1521277 , @erichkeane wrote:

> A different (perhaps silly) question is why 'W' isn't sufficient?  It 
> represents int64_t, which I wonder if is sufficient.


I had asked in a separate conversation not to change the signatures outside of 
opencl. I believe W would use 'long' on 64-bit targets that aren't Windows or 
X32. The builtins are defined by gcc as well so I wanted to avoid making them 
different as much as possible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62580/new/

https://reviews.llvm.org/D62580



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:9308
 case 'L':
   assert(!IsSpecial && "Can't use 'L' with 'W', 'N' or 'Z' modifiers");
   assert(HowLong <= 2 && "Can't have  modifier");

O should be mentioned here



Comment at: clang/lib/AST/ASTContext.cpp:9314
   // 'N' behaves like 'L' for all non LP64 targets and 'int' otherwise.
   assert(!IsSpecial && "Can't use two 'N', 'W' or 'Z' modifiers!");
   assert(HowLong == 0 && "Can't use both 'L' and 'N' modifiers!");

O should be mentioned here



Comment at: clang/lib/AST/ASTContext.cpp:9324
   // This modifier represents int64 type.
   assert(!IsSpecial && "Can't use two 'N', 'W' or 'Z'  modifiers!");
   assert(HowLong == 0 && "Can't use both 'L' and 'W' modifiers!");

O should be mentioned here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62580/new/

https://reviews.llvm.org/D62580



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

A different (perhaps silly) question is why 'W' isn't sufficient?  It 
represents int64_t, which I wonder if is sufficient.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62580/new/

https://reviews.llvm.org/D62580



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

'O' is an interesting choice.  Any real justification for it, or just "what was 
available"?  It definitely needs to be documented in the top of Builtins.def 
however.




Comment at: clang/lib/AST/ASTContext.cpp:9362
+case 'O':
+  assert(!IsSpecial && "Can't use two 'N', 'W', 'Z' or 'O' modifiers!");
+  assert(HowLong == 0 && "Can't use both 'L' and 'O' modifiers!");

Likely need to update the asserts in the other places this is used, such as 'Z'.

Also, add the 'O' to the documentation in Builtins.def


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62580/new/

https://reviews.llvm.org/D62580



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