On Fri, Oct 14, 2022 at 11:40 PM Miguel Ojeda <miguel.ojeda.sandonis@xxxxxxxxx> wrote: > > On Tue, Oct 11, 2022 at 7:16 PM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote: > > > > diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h > > index 898b3458b24a..9221302f6ae8 100644 > > --- a/include/linux/compiler_attributes.h > > +++ b/include/linux/compiler_attributes.h > > @@ -64,16 +64,10 @@ > > * compiler should see some alignment anyway, when the return value is > > * massaged by 'flags = ptr & 3; ptr &= ~3;'). > > * > > - * Optional: not supported by icc > > - * > > * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-assume_005faligned-function-attribute > > * clang: https://clang.llvm.org/docs/AttributeReference.html#assume-aligned > > */ > > -#if __has_attribute(__assume_aligned__) > > -# define __assume_aligned(a, ...) __attribute__((__assume_aligned__(a, ## __VA_ARGS__))) > > -#else > > -# define __assume_aligned(a, ...) > > -#endif > > +#define __assume_aligned(a, ...) __attribute__((__assume_aligned__(a, ## __VA_ARGS__))) > > Thanks for cleaning the conditional inclusion here. I double-checked > it is indeed available for both GCC and Clang current minimum versions > just in case: https://godbolt.org/z/PxaqeEdcE. > > > diff --git a/lib/zstd/common/compiler.h b/lib/zstd/common/compiler.h > > index f5a9c70a228a..c281a6430cd4 100644 > > --- a/lib/zstd/common/compiler.h > > +++ b/lib/zstd/common/compiler.h > > @@ -116,7 +116,7 @@ > > > > /* vectorization > > * older GCC (pre gcc-4.3 picked as the cutoff) uses a different syntax */ > > -#if !defined(__INTEL_COMPILER) && !defined(__clang__) && defined(__GNUC__) > > +#if !defined(__clang__) && defined(__GNUC__) > > # if (__GNUC__ == 4 && __GNUC_MINOR__ > 3) || (__GNUC__ >= 5) > > # define DONT_VECTORIZE __attribute__((optimize("no-tree-vectorize"))) > > # else > > These files come from upstream Zstandard -- should we keep those lines > to minimize divergence? > https://github.com/facebook/zstd/blob/v1.4.10/lib/common/compiler.h#L154. > > Commit e0c1b49f5b67 ("lib: zstd: Upgrade to latest upstream zstd > version 1.4.10") is the latest upgrade, and says: > > This patch is 100% generated from upstream zstd commit 20821a46f412 [0]. > > This patch is very large because it is transitioning from the custom > kernel zstd to using upstream directly. The new zstd follows upstreams > file structure which is different. Future update patches will be much > smaller because they will only contain the changes from one upstream > zstd release. > > So I think Nick would prefer to keep the changes as minimal as > possible with respect to upstream. > > Further reading seems to suggest this is the case, e.g. see this > commit upstream that introduces a space to match the kernel: > https://github.com/facebook/zstd/commit/b53da1f6f499f0d44c5f40795b080d967b24e5fa. > > > diff --git a/lib/zstd/compress/zstd_fast.c b/lib/zstd/compress/zstd_fast.c > > index 96b7d48e2868..800f3865119f 100644 > > --- a/lib/zstd/compress/zstd_fast.c > > +++ b/lib/zstd/compress/zstd_fast.c > > @@ -80,13 +80,6 @@ ZSTD_compressBlock_fast_generic( > > } > > > > /* Main Search Loop */ > > -#ifdef __INTEL_COMPILER > > - /* From intel 'The vector pragma indicates that the loop should be > > - * vectorized if it is legal to do so'. Can be used together with > > - * #pragma ivdep (but have opted to exclude that because intel > > - * warns against using it).*/ > > - #pragma vector always > > -#endif > > while (ip1 < ilimit) { /* < instead of <=, because check at ip0+2 */ > > size_t mLength; > > BYTE const* ip2 = ip0 + 2; > > Ditto: https://github.com/facebook/zstd/blob/v1.4.10/lib/compress/zstd_fast.c#L83. > > Apart from the zstd divergence which I am not sure about, everything > looks good to me! > > Reviewed-by: Miguel Ojeda <ojeda@xxxxxxxxxx> > > Cheers, > Miguel Thanks for your close review. I will drop zstd changes and send v3. -- Best Regards Masahiro Yamada
