New issue
Advanced search Search tips

Issue 812395 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 709716



Sign in to add a comment

[zlib] Remove stub x86_check on ARM builds

Project Member Reported by cavalcantii@chromium.org, Feb 14 2018

Issue description

After landing the crc32 patch + arm_features code, it may make sense to remove the stub (https://cs.chromium.org/chromium/src/third_party/zlib/simd_stub.c) that is being built for ARM.

 
Cc: cblume@chromium.org noel@chromium.org
Blockedon: 709716

Comment 3 by noel@chromium.org, Feb 15 2018

There was some recent / relevant discussion about keeping the stub or not on
https://chromium-review.googlesource.com/c/chromium/src/+/901043#message-8b1a57c0bd2bfcfb7443be1a3cc50af246943117

"It isn't that anyone plans to remove the x86 optimization. There is no plan to stop calling x86_check_features().
Instead, the plan was to remove the empty stub.

Right now the code is:
x86_check_features();
BUILD.gn:
if(x86) {
  sources += "x86_stub.cc" // which does the CPUID test
} else {
  sources += "empty_stub.cc" // has an empty x86_check_features() function
}

This could instead be:
#if defined(x86)
x86_check_features();
#endif

No need for an empty stub.

Although, we might actually keep the stub idea now that we also have ARM features.

Instead of:
#if defined(x86)
x86_check_features();
#elif defined(ARM)
arm_check_features();
#endif

It could be:
check_cpu_features();
BUILD.gn:
if(x86) {
  sources += "x86_check_cpu_features.cc"
} else if(arm) {
  sources += "arm_check_cpu_features.cc"
} else {
  sources += "empty_check_cpu_features.cc"
}"

"Thanks for clarifying.  Your summary concluded, that on second thoughts, we might need to keep the stub.  I also feel that the code (as you noted) is telling us that we maybe just need a check_cpu_features() system.  I don't think that even needs per-arch directories.  We recently released a cpu detection library [1], no per-arch directories in there I note.  Not suggesting we use it, it's more than we need, but it is interesting to read over their code from a design perspective.

If there is a bug about the stub (it's removal).  It might be good to get your notes above entered into said bug? TODO(somebody): bug filed, done.

[1] https://opensource.googleblog.com/2018/02/cpu-features-library.html "


Mike then added comments about how to handle the SIMD code.  I chatted to him to be sure he was talking about alder32_simd and crcr32_simd, etc (and yes he was), and not about cpu feature detection (correct).  His concerns were about how we organize our SIMD code, which, to be clear, has nothing directly to do with how we organize cpu_feature detection.

"I'll just keep saying this so that there's no doubt in anyone's mind about my opinion on this sort of thing... organizing code by target architecture is a bad idea, akin to keeping your left shoes all in one place and your right shoes separate in another.

If we're going to write platform specific code, the concrete variants for each logical routine should all be as close together as possible, whether in the same file with #define guards, or literally the same code using some sort of vectorization abstraction (GCC/Clang extensions, a template library, etc), or at worst in foo.cc, foo_sse2.cc, foo_neon.cc all in the same directory.  The single most important thing in writing platform specific code is to be able to see clearly the parallels between platforms and conversely each platform's distinguishing features, and that's easier the closer the code stays.  Let's keep our shoes in pairs."

Then later in https://chromium-review.googlesource.com/c/chromium/src/+/911228 we had comments specifically about how the stub works.

"In fact, we also blindly #include "x86.h" on all platforms as well just a few lines above. That should also not happen on all platforms.

"This fact is one of history.  The intel.simd patch initially had feature include guards when it was posted for review.  agl@ didn't like the include guard "noise", and in review asked the developers to remove all the include guard #defines, and so it was done.

">> That should also not happen on all platforms."

"Recall that the x86 stuff in intel.simd came with a stub for other ports, so that it could be included and used on any port.  Maybe the compilers on other ports spot that the feature ints are always 0 for them, and compile any code that queries them away, eg., the generated code for ...

   if (x86_enable_whatever) {
      // blah blah 
   }

would be thrown away by an optimizing compiler since x86_enable_whatever is always 0; the if body code can't ever be used, so the compiler figures out there's no need to keep that code.

The code could be guarded by feature of course, but we need to go re-add guards to all the intel.simd code.  Not sure which is better, re-adding guards or using a stub."

So in conclusion: might need the stub, might need to check that the stub code gets optimized out during compilation. etc.

Sign in to add a comment