[PATCH] D65529: [PowerPC] Use xxleqv to set all one vector IMM(-1).

[Jinsong Ji via Phabricator via llvm-commits]

jsji marked 5 inline comments as done.
jsji added a comment.

Thanks @steven.zhang and @wuzish.



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrFormats.td:1212
 
-class XX3Form_Zero<bits<6> opcode, bits<8> xo, dag OOL, dag IOL, string asmstr,
+class XX3Form_SameOp<bits<6> opcode, bits<8> xo, dag OOL, dag IOL, string asmstr,
               InstrItinClass itin, list<dag> pattern>
----------------
steven.zhang wrote:
> Can you commit another NFC patch to do the rename and the XX3Form_SetZero seems to be completely the same as this one. I have no idea why we have two forms. We need to remove it.
Sure, done in https://reviews.llvm.org/rL368856


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:1298
+  
+  def : Pat<(v4i32 (bitconvert (v16i8 immAllOnesV))),
+            (XXLEQVOnes)>;
----------------
jsji wrote:
> wuzish wrote:
> > Has immAllOnesV contained bitcast semantic? It's equal to ISD::isBuildVectorAllOnes.
> No, it doesn't.
I should say, `isBuildVectorAllOnes` can see through bitcast, the `bitconvert` here is mostly the trick to get complexity up, so that we can prefer this over other patterns.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:1298
+  
+  def : Pat<(v4i32 (bitconvert (v16i8 immAllOnesV))),
+            (XXLEQVOnes)>;
----------------
steven.zhang wrote:
> jsji wrote:
> > jsji wrote:
> > > wuzish wrote:
> > > > Has immAllOnesV contained bitcast semantic? It's equal to ISD::isBuildVectorAllOnes.
> > > No, it doesn't.
> > I should say, `isBuildVectorAllOnes` can see through bitcast, the `bitconvert` here is mostly the trick to get complexity up, so that we can prefer this over other patterns.
> You are matching the pattern for v4i32, but miss to catch the other type of pattern. i.e. v8i16 etc. We won't canonical all the vector type to v4i32 during the type legalize.  Even we do that,  it is better to add all the match rules in td file as we cannot depend on the legalization implementation.
> 
> Your patch cannot handle the other vector type except the v4i32.
> ```
> define dso_local <8 x i16> @_Z3foov() local_unnamed_addr #0 {
> entry:
>   ret <8 x i16> <i16 -1, i16 -1, i16 -1, i16 -1,i16 -1, i16 -1, i16 -1, i16 -1>
> }
> ```
> 
> We need add some pattern like:
> ```
> def : Pat<(v8i16 (bitconvert (v16i8 immAllOnesV))),
>             (v8i16 (COPY_TO_REGCLASS(XXLEQVOnes), VSRC))>;
> ...
> ```
Agree. Will do that.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:4079
               (v16i8 (COPY_TO_REGCLASS (XXSPLTIB imm:$A), VSRC))>;
-    def : Pat<(v16i8 immAllOnesV),
-              (v16i8 (COPY_TO_REGCLASS (XXSPLTIB 255), VSRC))>;
----------------
jsji wrote:
> wuzish wrote:
> > If we don't enum all the type, just use v4i32 version. Are other types bitcast/canonical into v4i32 automatically?
> We will canonize `isBuildVectorAllOnes` into v16i8, we don't need to enum all types.
Ah, my bad, yes, you are right, I mis-read the condition of lowering, we did not really canonize.


================
Comment at: llvm/test/CodeGen/PowerPC/build-vector-tests.ll:761
 ; P9BE:       # %bb.0: # %entry
-; P9BE-NEXT:    xxspltib v2, 255
+; P9BE-NEXT:    xxleqv v2, v2, v2
 ; P9BE-NEXT:    blr
----------------
steven.zhang wrote:
> Add some tests to test v1i128, v2i64, v8i16 v16i8
I will add those in another file, this file is far too big.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65529/new/

https://reviews.llvm.org/D65529