linux-kernel - Re: [PATCH v2] Remove Intel compiler support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAK7LNAQkSbH-Gw3sgMzxHyA9pSf3gLVvbCLg3yvbO43_vi5=YA@mail.gmail.com>
Date:   Mon, 17 Oct 2022 03:22:59 +0900
From:   Masahiro Yamada <masahiroy@...nel.org>
To:     Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc:     Nick Terrell <terrelln@...com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H . Peter Anvin" <hpa@...or.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-ia64@...r.kernel.org,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Miguel Ojeda <ojeda@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        Len Brown <lenb@...nel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Robert Moore <robert.moore@...el.com>,
        Tom Rix <trix@...hat.com>, devel@...ica.org,
        linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        llvm@...ts.linux.dev
Subject: Re: [PATCH v2] Remove Intel compiler support

On Fri, Oct 14, 2022 at 11:40 PM Miguel Ojeda
<miguel.ojeda.sandonis@...il.com> wrote:
>
> On Tue, Oct 11, 2022 at 7:16 PM Masahiro Yamada <masahiroy@...nel.org> 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@...nel.org>
>
> Cheers,
> Miguel


Thanks for your close review.

I will drop zstd changes and send v3.



-- 
Best Regards
Masahiro Yamada