[PATCH] D28574: [scudo] Refactor of CRC32 and ARM runtime CRC32 detection

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/scudo/scudo_allocator.cpp:82
+  // the hardware version of the CRC32.
+#if defined(__SSE4_2__)
+  return computeHardwareCRC32(Crc, Data);
----------------
mgorny wrote:
> rengolin wrote:
> > cryptoad wrote:
> > > mgorny wrote:
> > > > If I'm not mistaken, you could add `|| defined(__ARM_FEATURE_CRC32)` to cover the same idea for ARM.
> > > I think for ARM, since the feature could be enabled via -mcrc (specifically for CRC32, unlike the whole instruction set for SSE 4.2), the runtime check is still required, as no other potentially illegal instructions would be emitted anywhere else.
> > Right, this is actually a good point. There are basically two ideas here:
> > 
> > 1. You compile to force one implementation or the other, via flags. In this case, if you compile to have CRC, it will fail with `sigill` on cores that don't have it. The most likely scenario is that all builds will end up not having it, for safety, so you'll never use it.
> > 
> > 2. You have a run time check, as it is now, so that it's not a compilation option and you'll always use whenever you can. The problem now is that you end up paying the comparison price on every call.
> > 
> > The third, and best (but more complicated) way is to use IFUNC, but that's certainly for another patch.
> > 
> > FWIW, right now, the AARch64 sanitizers are getting the VMA configuration via run time calls, just like this patch, because of the problems outlines above, so this is not without prior art. :)
> However, the instruction can be enabled implicitly via `-march`. If you build purely for ARM target that supports CRC32, I don't see a point not to use it.
With ARM, the probability that you build in one machine (type) and run in another is higher than in x86. The number of variations and hardware support is also much much higher.

All x86_64 chips of family X have feature Y. A mix bag of ARM chips on family X, which support feature Y, has it in a way that is actually usable (ie. some cores report the feature but you really rather not use it).

All I'm saying is that, using -march=aarch64+crc will not work on any core that doesn't have it, so everyone will *not* use it in case the binary has to run on hardware that doesn't have it. Any Linux distribution (including Android) will not enable it for that reason.

If you're advocating for hard-coding if the feature IS there in mattr (either +crc or +nocrc), but leaving run-time if it's not, than I think that'll be ok, as it's a platform choice.


https://reviews.llvm.org/D28574