Re: [PATCH 2/2] arm: lib: xor-neon: disable clang vectorization

[Adrian Ratiu <adrian.ratiu@collabora.com>]

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