lkml.org 
[lkml]   [2020]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 2/2] arm: lib: xor-neon: disable clang vectorization
Date
On Tue, 10 Nov 2020, Nick Desaulniers <ndesaulniers@google.com> 
wrote:
> On Tue, Nov 10, 2020 at 3:54 PM Adrian Ratiu
> <adrian.ratiu@collabora.com> wrote:
>>
>> On Tue, 10 Nov 2020, Nick Desaulniers <ndesaulniers@google.com>
>> wrote:
>> > On Mon, Nov 9, 2020 at 11:51 AM Adrian Ratiu
>> > <adrian.ratiu@collabora.com> wrote:
>> >>
>> >> On Fri, 06 Nov 2020, Nick Desaulniers
>> >> <ndesaulniers@google.com> wrote:
>> >> > +#pragma clang loop vectorize(enable)
>> >> > do {
>> >> > p1[0] ^= p2[0] ^ p3[0] ^ p4[0] ^ p5[0];
>> >> > p1[1] ^= p2[1] ^ p3[1] ^ p4[1] ^ p5[1];
>> >> > ``` seems to generate the vectorized code.
>> >> >
>> >> > Why don't we find a way to make those pragma's more
>> >> > toolchain portable, rather than open coding them like I
>> >> > have above rather than this series?
>> >>
>> >> Hi again Nick,
>> >>
>> >> How did you verify the above pragmas generate correct
>> >> vectorized code? Have you tested this specific use case?
>> >
>> > I read the disassembly before and after my suggested use of
>> > pragmas; look for vld/vstr. You can also add
>> > -Rpass-missed=loop-vectorize to CFLAGS_xor-neon.o in
>> > arch/arm/lib/Makefile and rebuild arch/arm/lib/xor-neon.o
>> > with CONFIG_BTRFS enabled.
>> >
>> >>
>> >> I'm asking because overrulling the cost model might not be
>> >> enough, the only thing I can confirm is that the generated
>> >> code is changed, but not that it is correct in any way. The
>> >> object disasm also looks weird, but I don't have enough
>> >> knowledge to start debugging what's happening within
>> >> LLVM/Clang itself.
>> >
>> > It doesn't "look weird" to me. The loop is versioned based on
>> > a comparison whether the parameters alias or not. There's a
>> > non-vectorized version if the parameters are equal or close
>> > enough to overlap. There's another version of the loop
>> > that's vectorized. If you want just the vectorized version,
>> > then you have to mark the parameters as __restrict qualified,
>> > then check that all callers are ok with that.
>> >
>>
>> Thank you for the explanation, that does make sense now. I'm
>> just a compiler optimization noob, sorry. All your help is much
>> appreciated.
>
> Don't worry about it; you'll get the hang of it in no time, just
> stick with it.
>
>>
>> >>
>> >> I also get some new warnings with your code [1], besides the
>> >> previously 'vectorization was possible but not beneficial'
>> >> which is still present. It is quite funny because these two
>> >> warnings seem to contradict themselves. :)
>> >
>> > From which compiler? ``` $ clang
>> > -Wpass-failed=transform-warning -c -x c /dev/null warning:
>> > unknown warning option '-Wpass-failed=transform-warning'; did
>> > you mean '-Wprofile-instr-missing'?
>> > [-Wunknown-warning-option] ```
>>
>> I'm using Clang 10.0.1-1 from the Arch Linux repo.
>>
>> In the LLVM sources that transform-warning appears to be
>> documented under
>> llvm-10.0.1.src/docs/Passes.rst:1227:-transform-warning
>>
>> Here's a build log: http://ix.io/2DIc
>>
>> I always get those warnings with the pragma change you
>> suggested, even on clean builds on latest linux-next.
>>
>> I looked at the Arch PKGBUILD and they don't appear to do
>> anything special other than patching to enable SSP and PIE by
>> default (eg llvm bug 13410).
>
> Ah, custom builds of LLVM. Grepping for transform-warning in
> LLVM's sources, I can indeed see such a pass. I'm curious
> whether Arch is turning on that pass by default or if you
> manually enabled -Wpass-failed=transform-warning in the
> Makefile? Maybe I need to do an assertions enabled build of
> LLVM or a debug build. Reading through llvm/docs/Passes.rst and
> llvm/docs/TransformMetadata.rst, it sounds like this should be
> triggered when a "forced optimization has failed." So I wonder
> what's the missing variable between it working for me, vs
> warning for you?

I did not build clang myself, just did "pacman -S clang" to get
the official distro binary package. Here's the PKGBUILD they used,
I'm sending the commit link because recently clang 11 was upgraded
to.

I also tested clang 11.0.0 where I get the same warnings /
remarks.

https://github.com/archlinux/svntogit-packages/blob/8ff1bb4e4be5c6e5bede60c6b259a89f0cee6e6a/trunk/PKGBUILD

>
> Godbolt seems to agree with me here:
> https://godbolt.org/z/Wf6YKv. Maybe related to the "New Pass
> Manager" ... digging into that...
>
>>
>> >
>> > The pragma is clang specific, hence my recommendation to wrap
>> > it in an #ifdef __clang__.
>> >
>>
>> Yes, I understand that. :)
>>
>> >>
>> >> At this point I do not trust the compiler and am inclined to
>> >> do
>> >
>> > Nonsense.
>> >
>> >> like was done for GCC when it was broken: disable the
>> >> optimization and warn users to upgrade after the compiler is
>> >> fixed and confirmed to work.
>> >>
>> >> If you agree I can send a v2 with this and also drop the GCC
>> >> pragma as Arvind and Ard suggested.
>> >
>> > If you resend "this" as in 2/2, I will NACK it. There's
>> > nothing wrong with the cost model; it's saying there's little
>> > point in generating the vectorized version because you're
>> > still going to need a non-vectorized loop version anyways.
>> > Claiming there is a compiler bug here is dubious just because
>> > the cost models between two compilers differ slightly.
>>
>> Ok, so that "remark" from the compiler is safe to ignore.
>
> Are you always seeing it when building with the pragma's added,
> no change to CFLAGS_xor-neon.o in arch/arm/lib/Makefile?
>

No, I have to modify CFLAGS_xor-neon.o to see the remarks. If I do
a build with just the pragma change I only always get the
warnings, not remarks.

Here's a more complete log with -Rpass-missed='.*' in the
Makefile, maybe the other remarks in there will help shed some
light.

http://ix.io/2DMl

>>
>> >
>> > Resend the patch removing the warning, remove the GCC pragma,
>> > but if you want to change anything here for Clang, use
>> > `#pragma clang loop vectorize(enable)` wrapped in an `#ifdef
>> > __clang__`.
>> >
>>
>> Thanks for making the NACK clear, so the way forward is to
>> either use the pragma if I can figure out the new 'loop not
>> vectorized' warning (which might also be a red herring) or just
>> leave Clang as is. :)
>
> Yes, though additionally Arvind points out that this code is
> kind of curious if there was overlap; maybe the parameters
> should just be restrict-qualified.
>

For now I think I'll just re-send the GCC changes and leave the
Clang optimization as is, until we better understand what's
happening and what's the best way to enable it.

>>
>> >>
>> >> Kind regards,
>> >> Adrian
>> >>
>> >> [1]
>> >> ./include/asm-generic/xor.h:11:1: warning: loop not vectorized:
>> >> the optimizer was unable to perform the requested transformation;
>> >> the transformation might be disabled or specified as part of an
>> >> unsupported transformation ordering
>> >> [-Wpass-failed=transform-warning] xor_8regs_2(unsigned long bytes,
>> >> unsigned long *p1, unsigned long *p2)
>
> --
> Thanks,
> ~Nick Desaulniers

\
 
 \ /
  Last update: 2020-11-11 15:15    [W:0.118 / U:0.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site