[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

[Olivier Giroux via Phabricator via cfe-commits]

__simt__ added a comment.

I think Jonathan is asking whether there is a match in the gray areas.

The two cases people bring up most:

1. Unions, where the padding overlaps for all the possible active members.
2. Tail padding, up to the allocator granularity / alignment size.

If the implementation-specific builtins don't match on these, then maybe they 
should have different names, is his argument I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87974

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

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

[Olivier Giroux via Phabricator via cfe-commits]

__simt__ added a comment.

I've resumed looking at the library code.

How should I check for support? Is it going to be e.g. 
`__has_feature(__builtin_clear_padding)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87974

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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Olivier Giroux via Phabricator via cfe-commits]

__simt__ added a comment.

In D74918#1897636 , @kristof.beyls 
wrote:

> If these values are part of the C++ target platform ABI, it seems to me the 
> values for std::hardware_{constructive,destructive}_interference_size should 
> be set by whoever has the authority to decide C++ platform ABI for specific 
> platforms.
>  Assuming my thought in the previous sentence is correct; discussions on 
> which values to chose for 
> std::hardware_{constructive,destructive}_interference_size should happen in 
> whichever forums decide C++ platform ABI for the various platforms? (Maybe 
> for some platforms that forum might be clang-related fora like 
> reviews.llvm.org, but probably not for all platforms).
>  With my (probably limited) understanding of the requirements, it seems like 
> deriving std::hardware_{constructive,destructive}_interference_size from 
> actual cache line size on a specific micro-architecture doesn't seem to be 
> the right approach?


They will be in the library ABI, meaning the libc++ ABI.

It's valid for libstdc++ and libc++ to have different values there. If we wish, 
we could try for an alignment (no pun intended) on these values, but even then 
that's just between these two libraries.

Which is good and encouraging, because I don't know what forum we would have to 
go to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Olivier Giroux via Phabricator via cfe-commits]

__simt__ added a comment.

(//I assume I'm not seeing a code review being used to veto a C++ Standard 
feature, but actually the other points are the reason for the red flag.//)

I can see a desire for hyper-precise definitions to achieve the best possible 
performance, but we really need a healthy does of conservatism here.

The C++ values can't change as a result of selecting between microarchitecture 
variations that are supposed to link, it takes an ABI break to change these.

We specified two numbers here so we could conservatively set them (e.g. 
constructive: smallest; destructive: largest) if we want, but then they are 
fixed.

I think there's just two plausible answers for x86_64:

1. constructive=64, destructive=64 (the only plausible answer for X86 classic 
edition)
2. constructive=64, destructive=128 (reserve some room for line-pair designs)

Recapping: precision is nice but we need to fix this in the ABI so ultimate 
precision isn't required nor desirable; we should choose one of these pairs (or 
debate if another pair is viable that I'm not thinking of); then we should ship 
C++17.




Comment at: clang/lib/Basic/Targets/X86.cpp:1748
+// | Haswell|  64 | 
https://www.7-cpu.com/cpu/Haswell.html  
 |
+// | Bradwell   |  64 | 
https://www.7-cpu.com/cpu/Broadwell.html
 |
+// | Skylake (including skylake-avx512) |  64 | 
https://www.nas.nasa.gov/hecc/support/kb/skylake-processors_550.html "Cache 
Hierarchy"  
 |

Broadwell.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Olivier Giroux via Phabricator via cfe-commits]

__simt__ added a comment.

In D71726#1792852 , @yaxunl wrote:

> In D71726#1791904 , @jfb wrote:
>
> > This generally seems fine. Does it work on most backends? I want to make 
> > sure it doesn't fail in backends :)
>
>
> For x86_64, amdgcn, aarch64, armv7, mips64, it is translated to cmpxchg by 
> AtomicExpandPass and backends did codegen successfully.
>
> For hexagon, riscv32, it is translated to call of `__atomic_fetch_add_4` for 
> fadd float. This is concerning. Probably we need to add 
> `__atomic_fetch_{add|sub}_{f16|f32|f64}` ?


For systems that have load-link/store-conditional architectures, like ARM / PPC 
/ base RISC-V without extension, I would imagine that using a cmpxchg loop is 
much worse than simply doing the floating-point add/sub in the middle of the 
atomic mini-transaction. I'm sure that we want back-ends to be capable of 
implementing this better than what this pass is doing, even when they don't 
have "native" fp atomics.

You listed amdgcn... what does this do on nvptx?


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

https://reviews.llvm.org/D71726



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