Hi Segher,

on 2019/7/25 ÏÂÎç9:49, Segher Boessenkool wrote:
> Hi Kewen,
> 
> On Tue, Jul 23, 2019 at 02:28:28PM +0800, Kewen.Lin wrote:
>> --- a/gcc/config/rs6000/altivec.md
>> +++ b/gcc/config/rs6000/altivec.md
>> @@ -1666,6 +1666,60 @@
>>    "vrl<VI_char> %0,%1,%2"
>>    [(set_attr "type" "vecsimple")])
>>  
>> +;; Here these vrl<VI2>_and are for vrotr<mode>3 expansion.
>> +;; since SHIFT_COUNT_TRUNCATED is set as zero, to append one explicit
>> +;; AND to indicate truncation but emit vrl<VI_char> insn.
>> +(define_insn "vrlv2di_and"
>> +  [(set (match_operand:V2DI 0 "register_operand" "=v")
>> +	(and:V2DI
>> +	  (rotate:V2DI (match_operand:V2DI 1 "register_operand" "v")
>> +	     (match_operand:V2DI 2 "register_operand" "v"))
>> +	  (const_vector:V2DI [(const_int 63) (const_int 63)])))]
>> +  "VECTOR_UNIT_P8_VECTOR_P (V2DImode)"
>> +  "vrld %0,%1,%2"
>> +  [(set_attr "type" "vecsimple")])
> 
> "vrlv2di_and" is an a bit unhappy name, we have a "vrlv" intruction.
> Just something like "rotatev2di_something", maybe?
> 
> Do we have something similar for non-rotate vector shifts, already?  We
> probably should, so please keep that in mind for naming things.
> 
> "vrlv2di_and" sounds like you first do the rotate, and then on what
> that results in you do the and.  And that is what the pattern does,
> too.  But this is wrong: it should mask off all but the lower bits
> of operand 2, instead.
> 

Thanks for reviewing!

You are right, the name matches the pattern but not what we want.
How about the name trunc_vrl<mode>, first do the truncation on the
operand 2 then do the vector rotation.  I didn't find any existing
shifts with the similar pattern.

I've updated the name and associated pattern in the new patch.

>> +(define_insn "vrlv16qi_and"
>> +  [(set (match_operand:V16QI 0 "register_operand" "=v")
>> +	(and:V16QI
>> +	(rotate:V16QI (match_operand:V16QI 1 "register_operand" "v")
>> +	   (match_operand:V16QI 2 "register_operand" "v"))
>> +	(const_vector:V16QI [(const_int 7) (const_int 7)
>> +			    (const_int 7) (const_int 7)
>> +			    (const_int 7) (const_int 7)
>> +			    (const_int 7) (const_int 7)
>> +			    (const_int 7) (const_int 7)
>> +			    (const_int 7) (const_int 7)
>> +			    (const_int 7) (const_int 7)
>> +			    (const_int 7) (const_int 7)])))]
>> +  "VECTOR_UNIT_ALTIVEC_P (V16QImode)"
>> +  "vrlb %0,%1,%2"
>> +  [(set_attr "type" "vecsimple")])
> 
> All the patterns can be merged into one (using some code_iterator).  That
> can be a later improvement.
> 

I guess you mean mode_attr?
I did try to merge them since they look tedious.  But the mode_attr can't
contain either "[" or "(" inside, it seems can't be used for different const 
vector mappings.  Really appreciate that if you can show me some examples.

>> +;; Return 1 if op is a vector register that operates on integer vectors
>> +;; or if op is a const vector with integer vector modes.
>> +(define_predicate "vint_reg_or_const_vector"
>> +  (match_code "reg,subreg,const_vector")

> Hrm, I don't like this name very much.  Why is just vint_operand not
> enough for what you use this for?
> 

vint_operand isn't enough since the expander legalizes the const vector into
vector register, I'm unable to get the feeder (const vector) of the input 
register operand.


>> +      rtx imm_vec
>> +	= simplify_const_unary_operation (NEG, <MODE>mode, operands[2],
> 
> (The "=" goes on the previous line).

OK, thanks.

>> +      emit_insn (gen_vrl<mode>_and (operands[0], operands[1], rot_count));
>> +    }
>> +  DONE;
>> +})
> 
> Why do you have to emit as the "and" form here?  Emitting the "bare"
> rotate should work just as well here?

Yes, the emitted insn is exactly the same.

It follows Jakub's suggestion via
https://gcc.gnu.org/ml/gcc-patches/2019-07/msg01159.html

Append one explicit AND to indicate the truncation for the case 
!SHIFT_COUNT_TRUNCATED.  (sorry if the previous pattern misled.)

> 
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/vec_rotate-1.c
>> @@ -0,0 +1,46 @@
>> +/* { dg-options "-O3" } */
>> +/* { dg-require-effective-target powerpc_vsx_ok } */
> 
>> +/* { dg-final { scan-assembler {\mvrld\M} } } */
>> +/* { dg-final { scan-assembler {\mvrlw\M} } } */
>> +/* { dg-final { scan-assembler {\mvrlh\M} } } */
>> +/* { dg-final { scan-assembler {\mvrlb\M} } } */
> 
> You need to generate code for whatever cpu introduced those insns,
> if you expect those to be generated ;-)
> 
> vsx_ok isn't needed.
> 

Thanks for catching, update it with altivec_ok in new patch.
I think we can still have this guard?  since those instructions 
origin from isa 2.03.