[Bug target/125941] AArch64: Inefficient code generation for non-constant svbool_t initializer

[tnfchris at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=125941

--- Comment #3 from Tamar Christina <tnfchris at gcc dot gnu.org> ---
(In reply to Christopher Bazley from comment #2)
> > So if the ticket is about the spills then that's not an issue. if it's about adding PMOV support, then yes :)
> 
> The context is this email thread:
> https://inbox.sourceware.org/gcc-patches/CAFiYyc19Ks7-
> mK047MWUmF_mfTdX6KOfQBjutA8fjnhDAvTk7w@mail.gmail.com/
> 
> Richard Biener is saying that the CTOR was previously 'invalid', although
> the test is pre-existing and the test passes without the assertion that he
> wants to add. He is also saying that vec_initvnx16bibi must be implemented.
> 
> It sounds as though you are saying that there is nothing wrong with the code
> currently generated by GCC, despite the fact that vec_initvnx16bibi has not
> been implemented.

Correct, I don't think Richi is right here. Predicate registers are 1/8th the
size of a vector register. The register above at VNx16BI is fully packed.

there are no "bits to clear" and so the constructor was perfectly valid.

A possibly invalid one is

#include <arm_sve.h>

#define VECT_CSTN { -1, t (svpfalse ()) }

int __attribute__ ((noipa))
t (svbool_t)
{
  return -1;
}

svbool_t __attribute__ ((noipa))
func_init4 ()
{
  svbool_t temp = VECT_CSTN;
  return temp;
}

where in GIMPLE you'd have

  svbool_t temp;
  int _1;
  <signed-boolean:1> _2;

  <bb 2> [local count: 1073741824]:
  _1 = t ({ 0, ... });
  _2 = _1 != 0;
  temp_5 = {-1, _2};
  return temp_5;

But if you look at the generated code:

        addpl   x2, sp, #7
        addpl   x1, sp, #7
        pfalse  p3.b
        add     x2, x2, 16
        addpl   x0, sp, #6
        add     x1, x1, 16
        add     x0, x0, 16
        str     p3, [x1]
        mov     w1, 1
        strh    w1, [x2]
        csetm   w1, ne
        ldr     p3, [x2]
        str     p3, [x0]
        ldrh    w0, [x0]
        bfi     w0, w1, 1, 1
        addpl   x1, sp, #6
        add     x1, x1, 16
        strh    w0, [x1]
        ldr     p0, [x1]
        ldp     x29, x30, [sp]
        addvl   sp, sp, #1
        add     sp, sp, 16

it's using the stack to construct the predicate because it has to set 1 bit at
a time.
But the zero-ing is done by the initial

        pfalse  p3.b

so this is semantically correct as well.

In RTL we have

(insn 8 7 9 2 (set (reg:VNx16BI 105 [ temp_5 ])
        (const_vector:VNx16BI repeat [
                (const_int 0 [0])
            ])) "cops.c":14:12 discrim 1 -1
     (nil))

which is generated by this clear

        /* Inform later passes that the old value is dead.  */
        if (!cleared && !vector && REG_P (target) && maybe_gt (n_elts, 1u))
          {
            emit_move_insn (target, CONST0_RTX (mode));
            cleared = 1;
          }

so it was always clearing it.

> 
> Would adding PMOV support entail implementing vec_initvnx16bibi? For all
> targets, or only for targets with SVE2p1?

This is one where the semantics of the instruction kinda makes it hard.
PMOV is a data to predicate vector transfer.

But to make the register the ugliness here is in the fact that every element
has to be converted to BImode.  This is a case where the C style initializers
don't match up with the svbool_t type and so it's a bit YOLO.

For the above I think the best solution is to replace the expression before
expansion to one that produces a boolean vector from data.

So essentially

svbool_t __attribute__ ((noipa))
func_init5 ()
{
  svint8_t temp = VECT_CSTN;
  return svcmpne_n_s8 (svptrue_b8 (), temp, 0);
}

This would then trigger the new defined zero-ing semantics and so you get

func_init5:
        stp     x29, x30, [sp, -16]!
        pfalse  p0.b
        mov     x29, sp
        bl      t
        mvni    v30.2s, 0
        ptrue   p0.b, all
        adrp    x1, .LC0
        ldp     x29, x30, [sp], 16
        add     x1, x1, :lo12:.LC0
        ld1rqb  z31.b, p0/z, [x1]
        insr    z31.b, w0
        insr    z31.b, b30
        cmpne   p0.b, p0/z, z31.b, #0
        ret

so in GIMPLE I think we want to have 

  svint8_t temp;
  int _1;
  signed char _2;
  svbool_t _6;

  <bb 2> [local count: 1073741824]:
  _1 = t ({ 0, ... });
  _2 = (signed char) _1;
  temp_5 = {-1, _2};
  _6 = temp_5 != { 0, ... };
  return _6;

instead.