[PATCH] D31331: [mips][msa] Truncation of vector elements for instructions creating ISD::SHL, ISD::SRL or ISD::SRA nodes

[Simon Dardis via Phabricator via llvm-commits]

sdardis added a comment.

@efriedma was right about constant folding taking care of the constant case, so you can ignore my comments about that.

Some more comments inlined, mostly on the tests.



================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:1556-1560
+  SDValue ConstValue = DAG.getConstant(Vec.getScalarValueSizeInBits() - 1,
+                                       DL, ResEltTy);
+  SDValue SplatVec = getBuildVectorSplat(ResTy, ConstValue, BigEndian, DAG);
+
+  return DAG.getNode(ISD::AND, DL, ResTy, Vec, SplatVec);
----------------
efriedma wrote:
> sdardis wrote:
> > This hunk only covers the non-constant case. When the operand to a logical ISD vector node--which implicitly masks the lower bits--is a ConstantSDnode, we should instead reformulate the constant so that it only contains bits that the MSA instructions will look at. 
> Doesn't constant folding cover the constant case?
You right, it does. I assumed it wouldn't.


================
Comment at: test/CodeGen/Mips/msa/3r-s.ll:134
 
 ; CHECK: llvm_mips_sll_b_test:
+; CHECK: lw [[R2:\$[0-9]+]], %got(llvm_mips_sll_b_ARG2)
----------------
For the test cases you've changed, can you update the CHECK: <function name>: to CHECK-LABEL: ?


================
Comment at: test/CodeGen/Mips/msa/3r-s.ll:143
 ; CHECK: .size llvm_mips_sll_b_test
 ;
 @llvm_mips_sll_h_ARG1 = global <8 x i16> <i16 0, i16 1, i16 2, i16 3, i16 4, i16 5, i16 6, i16 7>, align 16
----------------
These extra ; can be removed in the changed cases as well.


================
Comment at: test/CodeGen/Mips/msa/3r-s.ll:643
 @llvm_mips_srl_d_ARG1 = global <2 x i64> <i64 0, i64 1>, align 16
+;
 @llvm_mips_srl_d_ARG2 = global <2 x i64> <i64 2, i64 3>, align 16
----------------
This can be removed.


================
Comment at: test/CodeGen/Mips/msa/shift_constant_pool.ll:4-5
+
+; RUN: llc -march=mips64el -mattr=+msa,+fp64 -relocation-model=pic < %s | FileCheck -check-prefix=CHECK-ALL -check-prefix=MIPS64 %s
+; RUN: llc -march=mipsel -mattr=+msa,+fp64 -relocation-model=pic < %s | FileCheck -check-prefix=CHECK-ALL -check-prefix=MIPS32 %s
+
----------------
Rather than using CHECK-ALL, you can just use ALL for brevity. The multiple -check-prefix s can be combined into --check-prefixes=ALL,MIPS64 / --check-prefixes=ALL,MIPS32


================
Comment at: test/CodeGen/Mips/msa/shift_constant_pool.ll:24
+; CHECK-ALL: .4byte 4                       # 0x4
+; CHECK-ALL: llvm_mips_sll_w_test_const_vec:
+; MIPS32: lw $[[R2:[0-9]+]], %got([[LABEL]])($[[R1:[0-9]+]])
----------------
This should ALL-LABEL: llvm_mips_sll_w_test_const_vec:


================
Comment at: test/CodeGen/Mips/msa/shift_constant_pool.ll:33-34
+; CHECK-ALL: st.w $w0, 0($[[R3]])
+; CHECK-ALL: .end llvm_mips_sll_w_test_const_vec
+;
+ at llvm_mips_srl_w_test_const_vec_res = global <4 x i32> zeroinitializer, align 16
----------------
These two lines are unnecessary.


================
Comment at: test/CodeGen/Mips/msa/shift_constant_pool.ll:52
+; CHECK-ALL: .4byte 8                       # 0x8
+; CHECK-ALL: llvm_mips_srl_w_test_const_vec:
+; MIPS32: lw $[[R2:[0-9]+]], %got([[LABEL]])($[[R1:[0-9]+]])
----------------
See my comments on llvm_mips_sll_w_test_const_vec about ALL-LABEL and the last two lines which can be dropped.


================
Comment at: test/CodeGen/Mips/msa/shift_constant_pool.ll:80
+; CHECK-ALL: .4byte 8                       # 0x8
+; CHECK-ALL: llvm_mips_sra_w_test_const_vec:
+; MIPS32: lw $[[R2:[0-9]+]], %got([[LABEL]])($[[R1:[0-9]+]])
----------------
See my comments on llvm_mips_sll_w_test_const_vec about ALL-LABEL and the last two lines which can be dropped.


https://reviews.llvm.org/D31331