Hi Modi,

Thanks for your patch! 

> Adding support for extv<mode> and extzv<mode> on aarch64 as described in 
> PR86901. I also changed
> extract_bit_field_using_extv to use gen_lowpart_if_possible instead of 
> gen_lowpart directly. Using
> gen_lowpart directly will fail with an ICE in building libgcc when the 
> compiler fails to successfully do so 
> whereas gen_lowpart_if_possible will bail out of matching this pattern 
> gracefully.

I did a quick bootstrap, this shows several failures like:

gcc/builtins.c:9427:1: error: unrecognizable insn:
 9427 | }
      | ^
(insn 212 211 213 24 (set (reg:SI 207)
        (zero_extract:SI (reg:SI 206)
            (const_int 26 [0x1a])
            (const_int 6 [0x6]))) "/gcc/builtins.c":9413:44 -1
     (nil))

The issue here is that 26+6 = 32 and that's not a valid ubfx encoding. Currently
cases like this are split into a right shift in aarch64.md around line 5569:

;; When the bit position and width add up to 32 we can use a W-reg LSR
;; instruction taking advantage of the implicit zero-extension of the X-reg.
(define_split
  [(set (match_operand:DI 0 "register_operand")
        (zero_extract:DI (match_operand:DI 1 "register_operand")
                         (match_operand 2
                           "aarch64_simd_shift_imm_offset_di")
                         (match_operand 3
                           "aarch64_simd_shift_imm_di")))]
  "IN_RANGE (INTVAL (operands[2]) + INTVAL (operands[3]), 1,
             GET_MODE_BITSIZE (DImode) - 1)
   && (INTVAL (operands[2]) + INTVAL (operands[3]))
       == GET_MODE_BITSIZE (SImode)"
  [(set (match_dup 0)
        (zero_extend:DI (lshiftrt:SI (match_dup 4) (match_dup 3))))]
  {
    operands[4] = gen_lowpart (SImode, operands[1]);
  }

However that only supports DImode, not SImode, so it needs to be changed to
be more general using GPI.

Your new extv patterns should replace the magic patterns above it:

;; -------------------------------------------------------------------
;; Bitfields
;; -------------------------------------------------------------------

(define_expand "<optab>"

These are the current extv/extzv patterns, but without a mode. They are no 
longer
used when we start using the new ones.

Note you can write <optab><mode> to combine the extzv and extz patterns.
But please add a comment mentioning the pattern names so they are easy to find!

Besides a bootstrap it is always useful to compile a large body of code with 
your change
(say SPEC2006/2017) and check for differences in at least codesize. If there is 
an increase
in instruction count then there may be more issues that need to be resolved.

> I'm looking through past mails and https://gcc.gnu.org/contribute.html which 
> details testing bootstrap.
> I'm building a cross-compiler (x64_aarch64) and the instructions don't 
> address that scenario. The GCC 
> cross-build is green and there's no regressions on the C/C++ tests (The 
> go/fortran etc. look like they 
> need additional infrastructure built on my side to work). Is there a workflow 
> for cross-builds or should I 
> aim to get an aarch64 native machine for full validation?

I find it easiest to develop on a many-core AArch64 server so you get much 
faster builds,
bootstraps and regression tests. Cross compilers are mostly useful if you want 
to test big-endian
or new architecture features which are not yet supported in hardware. You don't 
normally need
to test Go/Fortran/ADA etc unless your patch does something that would directly 
affect them.

Finally do you have a copyright assignment with the FSF?

Cheers,
Wilco

Reply via email to