[PATCH] x86/bitops: Improve code generation for __scanbit()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

There's no need for an alternative.  Use the same technique as arch_ffs() by
pre-loading the destination register using the caller chosen "zero case"
instead of -1.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <jbeulich@xxxxxxxx>
CC: Roger Pau Monné <roger@xxxxxxxxxxxxxx>
CC: Teddy Astie <teddy.astie@xxxxxxxxxx>

Slightly RFC.  I don't particualrly like forward-declaring ffsl(), but I don't
see another option while maintaining a non-tangled common/arch header split.

This change is definitely a win at a micro level (it's removing a CMOV
instruction from every case, and removing an alternative from every
BITS_PER_LONG case), but causes an awful lot of churn which makes it hard to
judge at a macro level:

  add/remove: 3/0 grow/shrink: 7/20 up/down: 525/-1143 (-618)
  Function                                     old     new   delta
  cpumask_any                                    -     153    +153
  sched_set_res                                  -     132    +132
  csched_schedule                             3691    3819    +128
  __next_node.constprop                          -      41     +41
  do_microcode_update                         1014    1038     +24
  memory_add                                  4036    4049     +13
  csched2_alloc_domdata                        476     487     +11
  numa_init_array                              153     161      +8
  find_non_smt                                 320     328      +8
  dump_execstate                               221     228      +7
  fixup_irqs.cold                               49      47      -2
  csched2_res_pick                            1351    1349      -2
  psr_set_val                                 2270    2264      -6
  vmx_sync_exit_bitmap                          73      65      -8
  numa_emulation                               487     479      -8
  __find_next_zero_bit                         145     137      -8
  sched_unit_migrate_finish                   1255    1246      -9
  get_free_buddy                              1014    1003     -11
  node_to_scrub                                533     517     -16
  heap_init_late                              1659    1643     -16
  dom0_compute_nr_pages                       1424    1408     -16
  sched_init_vcpu                             1811    1791     -20
  dump_numa                                    471     443     -28
  runq_tickle                                 1753    1721     -32
  dump_numa.cold                               977     942     -35
  null_unit_migrate                            743     655     -88
  get_outstanding_claims                       198      74    -124
  _assign_irq_vector                          1571    1431    -140
  null_unit_insert                             694     542    -152
  fixup_irqs                                  1655    1233    -422

3 functions get out-of-lined, which has a large knock-on effect.

cpumask_any() has absolutely no buisness being inline in the first place, but
ends up with 3 callsites here (two from fixup_irqs(), one from
_assign_irq_vector()).
---
 xen/arch/x86/include/asm/bitops.h | 33 +++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/include/asm/bitops.h 
b/xen/arch/x86/include/asm/bitops.h
index 557cee03baeb..43a553d45257 100644
--- a/xen/arch/x86/include/asm/bitops.h
+++ b/xen/arch/x86/include/asm/bitops.h
@@ -320,18 +320,35 @@ extern unsigned int __find_first_zero_bit(
 extern unsigned int __find_next_zero_bit(
     const unsigned long *addr, unsigned int size, unsigned int offset);
 
+static attr_const unsigned int ffsl(unsigned long x);
+
+/*
+ * Search for the first set bit in @val, returning @max if @val was 0.
+ * See arch_ffs() for safety discussions.
+ */
 static always_inline unsigned int __scanbit(unsigned long val, unsigned int 
max)
 {
+    unsigned int r;
+
+    /* If the compiler knows val is nonzero, use a plain ffsl(). */
+    if ( __builtin_constant_p(val > 0) && val > 0 )
+        return ffsl(val) - 1;
+
+    /*
+     * If the compiler knows max is BITS_PER_LONG, use TZCNT directly.  The
+     * destination register still needs pre-loading for pre-TZCNT CPUs which
+     * will execute the instruction as BSF.
+     */
     if ( __builtin_constant_p(max) && max == BITS_PER_LONG )
-        alternative_io("bsf %[in],%[out]; cmovz %[max],%k[out]",
-                       "rep; bsf %[in],%[out]",
-                       X86_FEATURE_BMI1,
-                       [out] "=&r" (val),
-                       [in] "r" (val), [max] "r" (max));
+        asm ( "tzcnt %[val], %q[res]"
+              : [res] "=r" (r)
+              : [val] "rm" (val), "[res]" (max) );
     else
-        asm ( "bsf %1,%0 ; cmovz %2,%k0"
-              : "=&r" (val) : "r" (val), "r" (max) );
-    return (unsigned int)val;
+        asm ( "bsf %[val], %q[res]"
+              : [res] "=r" (r)
+              : [val] "rm" (val), "[res]" (max) );
+
+    return r;
 }
 
 /**

base-commit: c4a09c112c391ffc1aea33afff49ffb28156fbdb
-- 
2.39.5