[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Richard Membarth via Phabricator via cfe-commits]

richardmembarth added a comment.

Thanks for clarification and merging!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D54258#1528872 , @richardmembarth 
wrote:

> Merging in two weeks is fine for me.


No worries. I have just committed this.

> My assumption was that accepted patches are merged into upstream in a timely 
> manner.
>  Maybe this is not how it works?

No this is a manual process. The author is expected to commit unless he/she 
doesn't have commit rights. Then it has to be requested explicitly.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362623: [Clang] Fix pretty printing of CUDA address spaces 
(authored by stulova, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54258?vs=173154=203195#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54258

Files:
  cfe/trunk/lib/AST/TypePrinter.cpp


Index: cfe/trunk/lib/AST/TypePrinter.cpp
===
--- cfe/trunk/lib/AST/TypePrinter.cpp
+++ cfe/trunk/lib/AST/TypePrinter.cpp
@@ -1805,17 +1805,19 @@
   case LangAS::opencl_private:
 break;
   case LangAS::opencl_constant:
-  case LangAS::cuda_constant:
 OS << "__constant";
 break;
   case LangAS::opencl_generic:
 OS << "__generic";
 break;
   case LangAS::cuda_device:
-OS << "__device";
+OS << "__device__";
+break;
+  case LangAS::cuda_constant:
+OS << "__constant__";
 break;
   case LangAS::cuda_shared:
-OS << "__shared";
+OS << "__shared__";
 break;
   default:
 OS << "__attribute__((address_space(";


Index: cfe/trunk/lib/AST/TypePrinter.cpp
===
--- cfe/trunk/lib/AST/TypePrinter.cpp
+++ cfe/trunk/lib/AST/TypePrinter.cpp
@@ -1805,17 +1805,19 @@
   case LangAS::opencl_private:
 break;
   case LangAS::opencl_constant:
-  case LangAS::cuda_constant:
 OS << "__constant";
 break;
   case LangAS::opencl_generic:
 OS << "__generic";
 break;
   case LangAS::cuda_device:
-OS << "__device";
+OS << "__device__";
+break;
+  case LangAS::cuda_constant:
+OS << "__constant__";
 break;
   case LangAS::cuda_shared:
-OS << "__shared";
+OS << "__shared__";
 break;
   default:
 OS << "__attribute__((address_space(";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Aaron Ballman via cfe-commits]

On Tue, Jun 4, 2019 at 5:37 AM Richard Membarth via Phabricator
 wrote:
>
> richardmembarth added a comment.
>
> Merging in two weeks is fine for me.
>
> My assumption was that accepted patches are merged into upstream in a timely 
> manner.
> Maybe this is not how it works?

Your assumption is correct. Usually, if you don't have commit
privileges yet, you mention it once your patch is accepted so that the
reviewers know you need it committed on your behalf. The commit
usually happens same- or next-day. However, because I am traveling, I
don't have the ability to watch build bots and revert on your behalf
if anything goes wrong, which explains the delay on my part.

Sorry for the hassle.

~Aaron

>
>
> Repository:
>   rC Clang
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D54258/new/
>
> https://reviews.llvm.org/D54258
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Richard Membarth via Phabricator via cfe-commits]

richardmembarth added a comment.

Merging in two weeks is fine for me.

My assumption was that accepted patches are merged into upstream in a timely 
manner.
Maybe this is not how it works?


Repository:
  rC Clang

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

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D54258#1524643 , @richardmembarth 
wrote:

> Do you know when this will be merged?


I apologize, I wasn't aware you needed this merged on your behalf. I normally 
would be happy to do so, but I'm currently traveling. Maybe @Anastasia or 
someone else can help, otherwise I will merge it when I get back to the office 
in two weeks.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Richard Membarth via Phabricator via cfe-commits]

richardmembarth added a comment.
Herald added a subscriber: ebevhan.
Herald added a project: clang.

Do you know when this will be merged?


Repository:
  rC Clang

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

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Aaron Ballman via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D54258#1297687, @richardmembarth wrote:

> > In https://reviews.llvm.org/D54258#1297191, @Anastasia wrote:
> > 
> > I agree with the change itself... but it's quite annoying that such things 
> > can't be tested. :(
>
> Yes, that's a pity :(


Yeah, it seems like trying to change this in the frontend would also be a fair 
amount of work.

> Is there anything missing so that this can be merged?

No, I think this truly is untestable without major work. Given how trivial this 
change is, I'm okay with it.


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Richard Membarth via Phabricator via cfe-commits]

richardmembarth added a comment.

> In https://reviews.llvm.org/D54258#1297191, @Anastasia wrote:
> 
> I agree with the change itself... but it's quite annoying that such things 
> can't be tested. :(

Yes, that's a pity :(

Is there anything missing so that this can be merged?


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In https://reviews.llvm.org/D54258#1296706, @richardmembarth wrote:

> CUDA maps `__shared__` internally also to `__attribute__((shared))`:
>
>   #define __annotate__(a) \
>   __attribute__((a))
>   #define __location__(a) \
>   __annotate__(a)
>   ...
>   #define __shared__ \
>   __location__(shared)
>
>
> My guess is that Clang does it just the same way and only converts to 
> `LangAS::cuda_shared` for code generation in `GetGlobalVarAddressSpace`:
>  https://clang.llvm.org/doxygen/CodeGenModule_8cpp_source.html#l03305
>  In contrast, OpenCL uses keywords that are mapped directly to 
> `LangAS::opencl_local` etc.


I agree with the change itself... but it's quite annoying that such things 
can't be tested. :(


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Richard Membarth via Phabricator via cfe-commits]

richardmembarth added a comment.

CUDA maps `__shared__` internally also to `__attribute__((shared))`:

  #define __annotate__(a) \
  __attribute__((a))
  #define __location__(a) \
  __annotate__(a)
  ...
  #define __shared__ \
  __location__(shared)

My guess is that Clang does it just the same way and only converts to 
`LangAS::cuda_shared` for code generation in `GetGlobalVarAddressSpace`:
https://clang.llvm.org/doxygen/CodeGenModule_8cpp_source.html#l03305
In contrast, OpenCL uses keywords that are mapped directly to 
`LangAS::opencl_local` etc.


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D54258#1295158, @richardmembarth wrote:

> There are external tools (e.g. hipacc ) that 
> generate Clang AST. This AST uses `LangAS` annotations and emits incorrect 
> memory space specifiers for CUDA when pretty-printed.


That's good to know!

> We would need a different frontend that annotates LangAS::cuda_shared.

Do you happen to know why this behaves the way it does? e.g., is the bug that 
the frontend is annotating incorrectly and forces codegen to work around it, 
and fixing the frontend to annotate properly lets us remove some workarounds 
and fixes your issue?


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Richard Membarth via Phabricator via cfe-commits]

richardmembarth added a comment.

There are external tools (e.g. hipacc ) that generate 
Clang AST. This AST uses `LangAS` annotations and emits incorrect memory space 
specifiers for CUDA when pretty-printed.


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D54258#1292129, @richardmembarth wrote:

> I think it's not so easy to provide such tests for CUDA.
>  CUDA memory space specifiers are implemented via attributes, e.g. `#define 
> __shared__ __attribute__((shared))`.
>  As a result of this, they are pretty-printed via a different code path.
>  In my example, I call `Ctx.getAddrSpaceQualType(QT, LangAS::cuda_shared)`, 
> which is then pretty-printed via the code above.
>  Any hints how to provide tests for this one?


If there's no way to trigger a different spelling that a user would see, why is 
this change needed?


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Richard Membarth via Phabricator via cfe-commits]

richardmembarth added a comment.

Same problem here: The CUDA memory space specifiers are represented via 
attributes, e.g. `CUDASharedAttr` and only converted in CodeGen to 
`LangAS::cuda_shared`.
We would need a different frontend that annotates `LangAS::cuda_shared`.


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

Could this be tested using diagnostics that prints the type? Like in 
test/SemaOpenCL/address-spaces.cl.


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Richard Membarth via Phabricator via cfe-commits]

richardmembarth added a comment.

I think it's not so easy to provide such tests for CUDA.
CUDA memory space specifiers are implemented via attributes, e.g. `#define 
__shared__ __attribute__((shared))`.
As a result of this, they are pretty-printed via a different code path.
In my example, I call `Ctx.getAddrSpaceQualType(QT, LangAS::cuda_shared)`, 
which is then pretty-printed via the code above.
Any hints how to provide tests for this one?


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Can you add tests for this change? We typically have these in Misc by passing 
`-ast-print`.


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Richard Membarth via Phabricator via cfe-commits]

richardmembarth created this revision.
richardmembarth added reviewers: Anastasia, aaron.ballman.
Herald added a subscriber: cfe-commits.

The current pretty-printer emits OpenCL-style memory spaces specifiers: 
__device, __constant, and __shared.
The correct CUDA memory space specifiers are: __device__, __constant__, and 
__shared__:
https://docs.nvidia.com/cuda/cuda-c-programming-guide/index.html#variable-memory-space-specifiers


Repository:
  rC Clang

https://reviews.llvm.org/D54258

Files:
  lib/AST/TypePrinter.cpp


Index: lib/AST/TypePrinter.cpp
===
--- lib/AST/TypePrinter.cpp
+++ lib/AST/TypePrinter.cpp
@@ -1738,17 +1738,19 @@
   case LangAS::opencl_private:
 break;
   case LangAS::opencl_constant:
-  case LangAS::cuda_constant:
 OS << "__constant";
 break;
   case LangAS::opencl_generic:
 OS << "__generic";
 break;
   case LangAS::cuda_device:
-OS << "__device";
+OS << "__device__";
+break;
+  case LangAS::cuda_constant:
+OS << "__constant__";
 break;
   case LangAS::cuda_shared:
-OS << "__shared";
+OS << "__shared__";
 break;
   default:
 OS << "__attribute__((address_space(";


Index: lib/AST/TypePrinter.cpp
===
--- lib/AST/TypePrinter.cpp
+++ lib/AST/TypePrinter.cpp
@@ -1738,17 +1738,19 @@
   case LangAS::opencl_private:
 break;
   case LangAS::opencl_constant:
-  case LangAS::cuda_constant:
 OS << "__constant";
 break;
   case LangAS::opencl_generic:
 OS << "__generic";
 break;
   case LangAS::cuda_device:
-OS << "__device";
+OS << "__device__";
+break;
+  case LangAS::cuda_constant:
+OS << "__constant__";
 break;
   case LangAS::cuda_shared:
-OS << "__shared";
+OS << "__shared__";
 break;
   default:
 OS << "__attribute__((address_space(";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits