Can this be a constexpr instead of static. If not it could be const whether you decide to have it as static or not.

I think I tried that, but I dont think it worked for the intrinsics datatype.

If you managed to make the above constexpr without running into compilation issues, you might be able to make this constexpr to push the computation to compile-time since in theory the mask is completely known at compile-time.

Same two comments for similar cases below as well.

Yes. Thats what I anted to do, but that did not work. I am also surprised that I have vreinterpretq_f32_u32 left here. Need to fix this, else compiler seg faults. Although in this case it probably did not, it seems.

One thing to keep in mind is to add "cc" to the clobber list if the instruction modifies the flag register. You have to check the reference manual for all of the instructions you are using as inline assembly in this PR to see if that's the case.

Good point.

I would personally mark as const all parameters that do not change. :) Making mistakes is easy and I think it's good practice to program defensively.

It seems to me what this does is take UINT_MAX (i.e. 0xFFFFFFFF == 4294967295) and store the equivalent IEEE-754 FP32 bit pattern that's closest to representing 4294967295.0f (i.e. 0x4F800000) into val[0]. Is that what you intended? If you wanted the 0xFFFFFFFF bit pattern you should either use vreinterpretq_f32_u32 or *(const float32x4_t*)(&mask_low).

Since the type is uint32x4_t I am not sure how meaningful it is to cast to a different pointer type. On the other hand use of intrinsics was resulting in seg fault. I just look the intrinsic up in arm_neon.h and it was doing just (float32x4_t), so I replaced that with it. Although not sure why the similar use of intrinsic elsewhere did not cause problem.

Interesting.

You can align tmp_values and use the @align property of store for a slightly faster execution.

Again "memory" here and everywhere else you are doing a read or write memory access."

I think it makes ordering requirement stringent and prevents compiler from doing any optimization, as that makes it look like a barrier, I think. Hence I did not do it.

I am not sure it is optional. I was under the impression that it is required for correct behavior.

@AshkanAliabadi, so this one seems to suggest that it prevents compiler from reordering loads/stores before "asm". https://stackoverflow.com/questions/14449141/the-difference-between-asm-asm-volatile-and-clobbering-memory. Basically it makes it assume that you dont know what memory gets written inside "asm" so dont reorder load/stores. But since address registers are linked in the asm I assume compiler understands what gets written? I was trying not to put constraint on compiler optimization.
Seems like modifying this to "rm" should be ok: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

@AshkanAliabadi, doesnt seem that I can get "rm" to work so just added "memory" to the clobbers list as you suggested.

asignas(32)

OK it seems it is how PyTorch prefers to do it probably a practice from before alignas was added to the language.

We have a store() and a loadu(). You can consider adding aligned versions of these functions for internal use in this class, and then use them where you have control over the alignment of data like here. Just a suggestion.

I dont know for sure if it makes perf difference. It probably avoids address alignment check, but my understanding is that if the address is aligned there is no perf penalty but if it is not then there is one cycle penalty. In x86 if you use aligned vs of load with unaligned address you get exception, not sure if thats the case here but either way it not known to me if this introduces performance penalty.

Does this modify the flags register. I think not, but if it does you need to add "cc" to the clobber list.

I will check. Thanks for this catch.

I am still not sure this is the correct portable usage. The fact that the compiler crashes on a use of vreinterpretq_f32_s32 and accepts (float32x4_t) does not necessarily mean that one is logically correct and the other is not. There are places online that have uses of vreinterpret_f32_u32, for instance here: https://skia.googlesource.com/skia/+/refs/heads/chrome/m54/src/opts/SkNx_neon.h#57, and logically speaking your usage looks like a reinterpret_cast and I am puzzled what the underlying issue is.

It seems that arm_neon implementations of vreinterpretq_f32_s8 on Clang indeed does use a C style cast, but GCC is not. For instance take a look arm_neon.h here, or here both of which seem to be GCC's implementation that is using an internal compiler intrinsic so this approach is at least not portable even if from a practical standpoint both iOS and recent versions of Android use Clang.

Do we have any tests covering this code path? Have those tests run and passed on Android and iPhone (considering that our CI doesn't run any tests on a phone)?

What do you think @dreiss?

The fact that the compiler crashes on a use of vreinterpretq_f32_s32 and accepts (float32x4_t) does not necessarily mean that one is logically correct and the other is not.

I agree, but that is not the argument here. Reason for crash is that there is some interaction between using intrinsics and optimization path in 32bit compilation.
android/ndk#1248
https://bugs.llvm.org/show_bug.cgi?id=45824

I tried infact to keep only that inline assembly that was needed and tried to use intrinsics everywhere else. However this is not about which intrinsics are ok or not. Depending on the optimization path compiler takes it crashes or does not. So I cannot pick and chose where to use intrinsics and where not.

It seems that arm_neon implementations of vreinterpretq_f32_s8 on Clang indeed does use a C style cast, but GCC is not. For instance take a look arm_neon.h here, or here both of which seem to be GCC's implementation that is using an internal compiler intrinsic so this approach is at least not portable even if from a practical standpoint both iOS and recent versions of Android use Clang.

That is a fair point and we can do internal runs and test this with GCC to if that is sufficient.

But without that, either we put compiler specific ifdefs and enable this only for clang or not do 32bit arm neon backend at all.

Yeah, I think it's still possible to build PyTorch for ARM with gcc (on Raspberry Pi, for example), so we should keep that working. I think a compiler-specific ifdef with a note about why is fine.

Well what this means is that we are only postponing this issue since whoever will port PyTorch to C++-17 in a year or two will have to deal with this. :) Anyhow I am OK with that if you and David are.

This is microarchitecture dependent but generally there is a performance benefit to aligned loads and stores.

You can also specify an alignment for the pointer passed in Rn, using the optional : parameter, which often speeds up memory accesses.

Reference: https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/coding-for-neon---part-1-load-and-stores

You can see that your particular use (i.e. VLDn {Dd, D(d+1), D(d+2), D(d+3)}, Rn) can accept three different, increasingly more strict, alignments in the table on VLDn's reference page.

Could you please try to use vld1.32 {%S0 - %V0} instead of naming registers d6 to d9?
Unfortunately this will work only with gcc, and not with clang.