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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
