On Tue, Sep 01, 2020 at 04:09:53PM +0800, luoxhu wrote:
> On 2020/9/1 01:04, Segher Boessenkool wrote:
> > For v a V4SI, x a SI, j some int,  what do we generate for
> >    v[j&3] = x;
> > ?
> > This should be exactly the same as we generate for
> >    vec_insert(x, v, j);
> > (the builtin does a modulo 4 automatically).
> 
> No, even with my patch "stxv 34,-16(1);stwx 5,9,6;lxv 34,-16(1)" generated 
> currently.

I think you should solve the problem in the generic case, then, since it
is (presumably) much more frequent.

> Is it feasible and acceptable to expand some kind of pattern in expander 
> directly without
> builtin transition?

I don't understand what you mean?

> I borrowed some of implementation from vec_extract.  For vec_extract, the 
> issue also exists:
> 
>            source:                             gimple:                        
>      expand:                                        asm:
> 1) i = vec_extract (v, n);  =>  i = __builtin_vec_ext_v4si (v, n);   => 
> {r120:SI=unspec[r118:V4SI,r119:DI] 134;...}     => slwi 9,6,2   vextuwrx 
> 3,9,2   
> 2) i = vec_extract (v, 3);  =>  i = __builtin_vec_ext_v4si (v, 3);   => 
> {r120:SI=vec_select(r118:V4SI,parallel)...}     =>  li 9,12      vextuwrx 
> 3,9,2
> 3) i = v[n%4];   =>  _1 = n & 3;  i = VIEW_CONVERT_EXPR<int[4]>(v)[_1];  =>   
>      ...        =>     stxv 34,-16(1);addi 9,1,-16; rldic 5,5,2,60; lwax 3,9,5
> 4) i = v[3];     =>          i = BIT_FIELD_REF <v, 32, 96>;              =>  
> {r120:SI=vec_select(r118:V4SI,parallel)...} => li 9,12;   vextuwrx 3,9,2 
> 
> Case 3) also couldn't handle the similar usage, and case 4) doesn't generate 
> builtin as expected,
> it just expand to vec_select by coincidence.  So does this mean both 
> vec_insert and vec_extract
> and all other similar vector builtins should use IFN as suggested by Richard 
> Biener, to match the
> pattern in gimple and expand both constant and variable index in expander?  
> Will this also be
> beneficial for other targets except power?  Or we should do that gradually 
> after this patch
> approved as it seems another independent issue?  Thanks:)

I don't think we should do that at all?  IFNs just complicate
everything, there is no added value here: this is just data movement!
We need to avoid the data aliasing some generic way.


Segher

Reply via email to