[PATCH] D107492: [clang] Replace asm with __asm__ in cuda header

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Re: `__volatile__`. I've done some background reading on it but can't find the original motivation. `__asm__` exists because the asm word is in the application namespace and asm is an extension. Volatile has been with us since C89 though, and is also accepted under -ansi. I haven't been able to find a set of compiler flags that reject it.

Cuda however uses volatile to mean a different thing to C as it is part of the atomic model. From poking at `-x cuda` in godbolt it looks like clang lowers cuda volatile to IR volatile, and not to relaxed atomic. I'm not confident that's correct, in particular i++ on a volatile variable turns into a load and a store, not an atomicrmw. That's orthogonal to this particular change but does make me reluctant to touch the word 'volatile' in anything that is compiled as cuda.

Therefore I'd like to leave it as `__asm__ volatile`.



================
Comment at: clang/lib/Headers/__clang_cuda_device_functions.h:37
 #if defined(__cplusplus)
-__DEVICE__ void __brkpt() { asm volatile("brkpt;"); }
+__DEVICE__ void __brkpt() { __asm__ volatile("brkpt;"); }
 __DEVICE__ void __brkpt(int __a) { __brkpt(); }
----------------
tra wrote:
> Should we also change `volatile` ->  `__volatile__` here in other places in the file?
Replying at the end


================
Comment at: clang/lib/Headers/__clang_cuda_device_functions.h:1043
 }
-#else // CUDA_VERSION >= 9020
+#else  // CUDA_VERSION >= 9020
 // CUDA no longer provides inline assembly (or bitcode) implementation of these
----------------
emankov wrote:
> Unneeded formatting.
Correct formatting though. This is what clang-format did to the whole file. I'll revert the space in favour of git-clang-format if you prefer


================
Comment at: clang/lib/Headers/__clang_cuda_device_functions.h:1057
+  __asm__("vabsdiff2.s32.s32.s32 %0,%1,%2,%3;"
+          : "=r"(r)
+          : "r"(__a), "r"(0), "r"(0));
----------------
emankov wrote:
> Tabs are not allowed, please use whitespaces instead.
I've been there! It's not a tab,  that's the symbol phabricator unhelpfully uses to indicate whitespace change. In this case, four extra spaces to keep the : lined up with the brace


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107492