Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On 03/03/21 09:14 -0800, Thiago Macieira via Libstdc++ wrote: On Wednesday, 3 March 2021 06:34:26 PST Jonathan Wakely wrote: On 26/02/21 07:59 -0800, Thiago Macieira via Libstdc++ wrote: >ints can be used as futex on Linux. ptrdiff_t on 64-bit Linux can't. Yes, we should do this for GCC 11. Want me to re-submit this one alone, with the "alignas(4)" with a commend indicating it's what the kernel requires? Yes please.
Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On Wednesday, 3 March 2021 06:34:26 PST Jonathan Wakely wrote: > On 26/02/21 07:59 -0800, Thiago Macieira via Libstdc++ wrote: > >ints can be used as futex on Linux. ptrdiff_t on 64-bit Linux can't. > > Yes, we should do this for GCC 11. Want me to re-submit this one alone, with the "alignas(4)" with a commend indicating it's what the kernel requires? -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel DPG Cloud Engineering
Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On Wed, 3 Mar 2021, Jonathan Wakely wrote: > For int, there shouldn't be any need to force the alignment. I don't > think any ABI supported by GCC allows int members to be aligned to > less than __alignof__(int). (sizeof(int) last) The CRIS ABI does as in default packed, and ISTR there was some other gcc port too, but I don't recall whether that (too) had no (current) Linux port. brgds, H-P
Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On 03/03/21 14:56 +, Jonathan Wakely wrote: On 01/03/21 09:56 +0100, Richard Biener via Libstdc++ wrote: On Sun, Feb 28, 2021 at 10:53 PM Hans-Peter Nilsson wrote: On Fri, 26 Feb 2021, Thiago Macieira via Gcc-patches wrote: On Friday, 26 February 2021 11:31:00 PST Andreas Schwab wrote: > On Feb 26 2021, Thiago Macieira wrote: > > On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: > >> On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: > >> > -alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; > >> > +alignas(__alignof__(int)) int _M_a; > >> > >> Futexes must be aligned to 4 bytes. > > > > Agreed, but doesn't this accomplish that? > > No. It uses whatever alignment the type already has, and is an > elaborate no-op. I thought so too when I read the original line. But I expected it was written like that for a reason, especially since the same pattern appears in other places. I can change to "alignas(4)" (which is a GCC extension, I believe). Is that the correct solution? It's not a GCC extensions. You're thinking of alignas(obj) which is a GCC extension. IMNSHO make use of the corresponding atomic type. Then there'd be no need for separate what's-the-right-align-curse games. That won't work though, because we need direct access to the integer object, not to a std::atomic which contains it. Or align as the corresponding atomic type (in case using an actual std::atomic is undesirable). OTOH the proposed code isn't any more bogus than the previous ... The previous code accounts for the fact that ptrdiff_t is a typedef for an unspecified type, and that some ABIs allow struct members to have weaker alignment than they would have otherwise. e.g. __alignof__(long long) != alignof(long long) on x86. Yes, I know ptrdiff_t isn't long long on x86, but since it's a typedef for some target-specific type, it's still possible that alignof != __alignof__. So alignas(__alignof__(T)) is not necessarily a no-op. So not bogus. For int, there shouldn't be any need to force the alignment. I don't think any ABI supported by GCC allows int members to be aligned to less than __alignof__(int). Users could break it by compiling with #pragma pack, but I have no sympathy for such silliness. Jakub said on IRC that m68k might have alignof(int) == 2, so we'd need to increase that alignment to use it as a futex. N.B. std::atomic and std::atomic_ref don't use alignas(__alignof__(T)) they do this instead: static_assert(is_trivially_copyable_v<_Tp>); // 1/2/4/8/16-byte types must be aligned to at least their size. static constexpr int _S_min_alignment = (sizeof(_Tp) & (sizeof(_Tp) - 1)) || sizeof(_Tp) > 16 ? 0 : sizeof(_Tp); public: static constexpr size_t required_alignment = _S_min_alignment > alignof(_Tp) ? _S_min_alignment : alignof(_Tp); Using std::atomic_ref::required_alignment would give that value. For something being used as a futex we should just use alignas(4) though, since that's what the kernel requires.
Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On Mär 03 2021, Jonathan Wakely wrote: > For int, there shouldn't be any need to force the alignment. I don't > think any ABI supported by GCC allows int members to be aligned to > less than __alignof__(int). There is no requirement that __alignof__(int) is big enough. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On 01/03/21 09:56 +0100, Richard Biener via Libstdc++ wrote: On Sun, Feb 28, 2021 at 10:53 PM Hans-Peter Nilsson wrote: On Fri, 26 Feb 2021, Thiago Macieira via Gcc-patches wrote: > On Friday, 26 February 2021 11:31:00 PST Andreas Schwab wrote: > > On Feb 26 2021, Thiago Macieira wrote: > > > On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: > > >> On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: > > >> > -alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; > > >> > +alignas(__alignof__(int)) int _M_a; > > >> > > >> Futexes must be aligned to 4 bytes. > > > > > > Agreed, but doesn't this accomplish that? > > > > No. It uses whatever alignment the type already has, and is an > > elaborate no-op. > > I thought so too when I read the original line. But I expected it was written > like that for a reason, especially since the same pattern appears in other > places. > > I can change to "alignas(4)" (which is a GCC extension, I believe). Is that > the correct solution? IMNSHO make use of the corresponding atomic type. Then there'd be no need for separate what's-the-right-align-curse games. That won't work though, because we need direct access to the integer object, not to a std::atomic which contains it. Or align as the corresponding atomic type (in case using an actual std::atomic is undesirable). OTOH the proposed code isn't any more bogus than the previous ... The previous code accounts for the fact that ptrdiff_t is a typedef for an unspecified type, and that some ABIs allow struct members to have weaker alignment than they would have otherwise. e.g. __alignof__(long long) != alignof(long long) on x86. Yes, I know ptrdiff_t isn't long long on x86, but since it's a typedef for some target-specific type, it's still possible that alignof != __alignof__. So alignas(__alignof__(T)) is not necessarily a no-op. So not bogus. For int, there shouldn't be any need to force the alignment. I don't think any ABI supported by GCC allows int members to be aligned to less than __alignof__(int). Users could break it by compiling with #pragma pack, but I have no sympathy for such silliness.
Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On 26/02/21 07:59 -0800, Thiago Macieira via Libstdc++ wrote:
ints can be used as futex on Linux. ptrdiff_t on 64-bit Linux can't.
Yes, we should do this for GCC 11.
libstdc++-v3/include/std/latch | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libstdc++-v3/include/std/latch b/libstdc++-v3/include/std/latch
index ef8c301e5e9..156aea5c5e5 100644
--- a/libstdc++-v3/include/std/latch
+++ b/libstdc++-v3/include/std/latch
@@ -48,7 +48,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
public:
static constexpr ptrdiff_t
max() noexcept
-{ return __gnu_cxx::__int_traits::__max; }
+{ return __gnu_cxx::__int_traits::__max; }
constexpr explicit latch(ptrdiff_t __expected) noexcept
: _M_a(__expected) { }
@@ -85,7 +85,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
}
private:
-alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a;
+alignas(__alignof__(int)) int _M_a;
};
_GLIBCXX_END_NAMESPACE_VERSION
} // namespace
--
2.30.1
Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On 2021-02-28 13:31, Hans-Peter Nilsson wrote: On Fri, 26 Feb 2021, Thiago Macieira via Gcc-patches wrote: On Friday, 26 February 2021 11:31:00 PST Andreas Schwab wrote: On Feb 26 2021, Thiago Macieira wrote: On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: -alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; +alignas(__alignof__(int)) int _M_a; Futexes must be aligned to 4 bytes. Agreed, but doesn't this accomplish that? No. It uses whatever alignment the type already has, and is an elaborate no-op. I thought so too when I read the original line. But I expected it was written like that for a reason, especially since the same pattern appears in other places. I can change to "alignas(4)" (which is a GCC extension, I believe). Is that the correct solution? IMNSHO make use of the corresponding atomic type. Then there'd be no need for separate what's-the-right-align-curse games. There is no predicate wait on atomic. brgds, H-P
Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On Sun, Feb 28, 2021 at 10:53 PM Hans-Peter Nilsson wrote: > > > > On Fri, 26 Feb 2021, Thiago Macieira via Gcc-patches wrote: > > > On Friday, 26 February 2021 11:31:00 PST Andreas Schwab wrote: > > > On Feb 26 2021, Thiago Macieira wrote: > > > > On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: > > > >> On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: > > > >> > -alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; > > > >> > +alignas(__alignof__(int)) int _M_a; > > > >> > > > >> Futexes must be aligned to 4 bytes. > > > > > > > > Agreed, but doesn't this accomplish that? > > > > > > No. It uses whatever alignment the type already has, and is an > > > elaborate no-op. > > > > I thought so too when I read the original line. But I expected it was > > written > > like that for a reason, especially since the same pattern appears in other > > places. > > > > I can change to "alignas(4)" (which is a GCC extension, I believe). Is that > > the correct solution? > > IMNSHO make use of the corresponding atomic type. Then there'd > be no need for separate what's-the-right-align-curse games. Or align as the corresponding atomic type (in case using an actual std::atomic is undesirable). OTOH the proposed code isn't any more bogus than the previous ... Richard. > brgds, H-P
Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On Fri, 26 Feb 2021, Thiago Macieira via Gcc-patches wrote: > On Friday, 26 February 2021 11:31:00 PST Andreas Schwab wrote: > > On Feb 26 2021, Thiago Macieira wrote: > > > On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: > > >> On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: > > >> > -alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; > > >> > +alignas(__alignof__(int)) int _M_a; > > >> > > >> Futexes must be aligned to 4 bytes. > > > > > > Agreed, but doesn't this accomplish that? > > > > No. It uses whatever alignment the type already has, and is an > > elaborate no-op. > > I thought so too when I read the original line. But I expected it was written > like that for a reason, especially since the same pattern appears in other > places. > > I can change to "alignas(4)" (which is a GCC extension, I believe). Is that > the correct solution? IMNSHO make use of the corresponding atomic type. Then there'd be no need for separate what's-the-right-align-curse games. brgds, H-P
Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On Friday, 26 February 2021 11:31:00 PST Andreas Schwab wrote: > On Feb 26 2021, Thiago Macieira wrote: > > On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: > >> On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: > >> > -alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; > >> > +alignas(__alignof__(int)) int _M_a; > >> > >> Futexes must be aligned to 4 bytes. > > > > Agreed, but doesn't this accomplish that? > > No. It uses whatever alignment the type already has, and is an > elaborate no-op. I thought so too when I read the original line. But I expected it was written like that for a reason, especially since the same pattern appears in other places. I can change to "alignas(4)" (which is a GCC extension, I believe). Is that the correct solution? -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel DPG Cloud Engineering
Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On Feb 26 2021, Thiago Macieira wrote: > On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: >> On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: >> > @@ -85,7 +85,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> > } >> > >> > private: >> > -alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; >> > +alignas(__alignof__(int)) int _M_a; >> >> Futexes must be aligned to 4 bytes. > > Agreed, but doesn't this accomplish that? No. It uses whatever alignment the type already has, and is an elaborate no-op. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On Friday, 26 February 2021 10:14:42 PST Andreas Schwab wrote: > On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: > > @@ -85,7 +85,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > } > > > > private: > > -alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; > > +alignas(__alignof__(int)) int _M_a; > > Futexes must be aligned to 4 bytes. Agreed, but doesn't this accomplish that? -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel DPG Cloud Engineering
Re: [PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
On Feb 26 2021, Thiago Macieira via Gcc-patches wrote: > @@ -85,7 +85,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > >private: > -alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a; > +alignas(__alignof__(int)) int _M_a; Futexes must be aligned to 4 bytes. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
[PATCH 1/5] std::latch: reduce internal implementation from ptrdiff_t to int
ints can be used as futex on Linux. ptrdiff_t on 64-bit Linux can't.
---
libstdc++-v3/include/std/latch | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libstdc++-v3/include/std/latch b/libstdc++-v3/include/std/latch
index ef8c301e5e9..156aea5c5e5 100644
--- a/libstdc++-v3/include/std/latch
+++ b/libstdc++-v3/include/std/latch
@@ -48,7 +48,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
public:
static constexpr ptrdiff_t
max() noexcept
-{ return __gnu_cxx::__int_traits::__max; }
+{ return __gnu_cxx::__int_traits::__max; }
constexpr explicit latch(ptrdiff_t __expected) noexcept
: _M_a(__expected) { }
@@ -85,7 +85,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
}
private:
-alignas(__alignof__(ptrdiff_t)) ptrdiff_t _M_a;
+alignas(__alignof__(int)) int _M_a;
};
_GLIBCXX_END_NAMESPACE_VERSION
} // namespace
--
2.30.1
