On Fri, Jul 26, 2019 at 11:20:27AM +0800, Kewen.Lin wrote:
> on 2019/7/25 下午9:49, Segher Boessenkool wrote:
> > "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.
>
> 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.
vector.md has define_expand "vrotl<mode>3". Can you use that one
directly?
> > All the patterns can be merged into one (using some code_iterator). That
> > can be a later improvement.
>
> I guess you mean mode_attr?
code iterator, mode iterator, what's the difference ;-) (Yup you're right
of course).
> 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.
You might need to define a new predicate? I'll have a look later.
> >> +;; 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.
Hrm. Ideally you wouldn't expand to such very specific patterns in the
first place.
> > 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.)
This is rs6000-specific code. We always have !SHIFT_COUNT_TRUNCATED, but
we *do* in all cases have similar effects (it just differs per mode and
on the exact operation exactly how many bits are masked).
> >> +/* { 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.
The ISA doc says "2.03" for the altivec insns, because that was the first
ISA version that describes altivec. Before this it was a separate doc;
altivec is as old as ISA 1.xx.
vrld needs power8, as you discovered. That means you need to have two
testcases, one you compile with -mdejagnu-cpu=power8, one without.
Segher
