[PowerPC] Set v1i128 to expand for SETCC to avoid crash

[ZhangKang]

Summary

PPC only supports the instruction selection for v16i8, v8i16, v4i32, v2i64, v4f32 and v2f64 for ISD::SETCC,
don't support the v1i128, so below IR will crash:

t35: v1i128 = setcc t47, t74, setult:ch

This patch is to set v1i128 to expand for SETCC to avoid crash. And the expanding can get the expected result.

[lkail]

llvm/test/CodeGen/PowerPC/setcc-vector.ll

4

Can you add RUN lines for pwr8 since it's guarded by hasP8Altivec?

[ZhangKang]

Add more test cases.

[steven.zhang]

Your test didn't crash even without your fix.

[ZhangKang]

In D84238#2165867, @steven.zhang wrote:

Your test didn't crash even without your fix.

It needs ASSERT on. I have tried it again, it will hit below assertion error:

Invalid integer vector compare condition
UNREACHABLE executed at /home/ken/llvm/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4044!

[lkail]

llvm/test/CodeGen/PowerPC/setcc-vector.ll

7

Should remove le or it will confuse developers since pwr7 doesn't feature little-endian.

[ZhangKang]

Remove le for PWR7.

[steven.zhang]

I still think there is something missing here. What you set the legalize action for SETCC is under P8 altivec, but why the case passed for P7 ? And this case passed without your patch :

define <1 x i1> @setcc_v1i128(<1 x i128> %a) {
entry:
  %0 = icmp ult <1 x i128> %a, <i128 35708>
  ret <1 x i1> %0
}

The only difference I see is that, it is v1i1 = setcc t47, t74, setult:ch instead of v1i128 = setcc t47, t74, setult:ch. So, what happens here ?

[ZhangKang]

In D84238#2165944, @steven.zhang wrote:

I still think there is something missing here. What you set the legalize action for SETCC is under P8 altivec, but why the case passed for P7 ? And this case passed without your patch :

P7 will convert result from vector v1i128 setcc to two scalar setcc i64.

define <1 x i1> @setcc_v1i128(<1 x i128> %a) {
entry:
  %0 = icmp ult <1 x i128> %a, <i128 35708>
  ret <1 x i1> %0
}

The only difference I see is that, it is v1i1 = setcc t47, t74, setult:ch instead of v1i128 = setcc t47, t74, setult:ch. So, what happens here ?

If the return value is <1 x i1>, the result will be converted to scalar i1 = setcc i128.
Using below IR, you will get the same assertion error this patch mentioned. Because the return value is <1 x i64>,
the result will not be converted to scalar.

define <1 x i64> @setcc_v1i128(<1 x i128> %a) {
entry:
  %0 = icmp ult <1 x i128> %a, <i128 35708>
  %1 = zext <1 x i1> %0 to <1 x i64>
  ret <1 x i64> %1
}

[steven.zhang]

In D84238#2166241, @ZhangKang wrote:

In D84238#2165944, @steven.zhang wrote:

I still think there is something missing here. What you set the legalize action for SETCC is under P8 altivec, but why the case passed for P7 ? And this case passed without your patch :

P7 will convert result from vector v1i128 setcc to two scalar setcc i64.

define <1 x i1> @setcc_v1i128(<1 x i128> %a) {
entry:
  %0 = icmp ult <1 x i128> %a, <i128 35708>
  ret <1 x i1> %0
}

The only difference I see is that, it is v1i1 = setcc t47, t74, setult:ch instead of v1i128 = setcc t47, t74, setult:ch. So, what happens here ?

If the return value is <1 x i1>, the result will be converted to scalar i1 = setcc i128.
Using below IR, you will get the same assertion error this patch mentioned. Because the return value is <1 x i64>,
the result will not be converted to scalar.

define <1 x i64> @setcc_v1i128(<1 x i128> %a) {
entry:
  %0 = icmp ult <1 x i128> %a, <i128 35708>
  %1 = zext <1 x i1> %0 to <1 x i64>
  ret <1 x i64> %1
}

The new test makes more sense to me. Can you please use this one ? So, yes, mark the v1i128 as expand so that, we won't have any setcc with v1i128 after type legalization, which fix the crashing you see when we are trying to select an opcode for v1i128.

[ZhangKang]

Update the test case.

[ZhangKang]

The new test makes more sense to me. Can you please use this one ? So, yes, mark the v1i128 as expand so that, we won't have any setcc with v1i128 after type legalization, which fix the crashing you see when we are trying to select an opcode for v1i128.

Have updated the test case.

[ZhangKang]

Remove the hasP8Vector condition.

[steven.zhang]

LGTM now. Thank you for fixing this.

[hans]

Pushed to 11.x as 7569e8c696288cd9c9409936b4fe9b846d0bd0b7 to fix https://bugs.llvm.org/show_bug.cgi?id=47374

Please let me know if there are any follow-ups.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

if (Subtarget.hasVSX()) {
         // 128 bit shifts can be accomplished via 3 instructions for SHL and
         // SRL, but not for SRA because of the instructions available:
         // VS{RL} and VS{RL}O. However due to direct move costs, it's not worth
         // doing
         setOperationAction(ISD::SHL, MVT::v1i128, Expand);
         setOperationAction(ISD::SRL, MVT::v1i128, Expand);
         setOperationAction(ISD::SRA, MVT::v1i128, Expand);
 
+        setOperationAction(ISD::SETCC, MVT::v1i128, Expand);
         setOperationAction(ISD::SETCC, MVT::v2i64, Legal);
       }
       else {
         setOperationAction(ISD::SHL, MVT::v2i64, Expand);
         setOperationAction(ISD::SRA, MVT::v2i64, Expand);
         setOperationAction(ISD::SRL, MVT::v2i64, Expand);
 
         setOperationAction(ISD::SETCC, MVT::v2i64, Custom);

llvm/test/CodeGen/PowerPC/setcc-vector.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mcpu=pwr9 -mtriple=powerpc64le-unknown-unknown \
+; RUN:   -ppc-asm-full-reg-names < %s | FileCheck %s
+
+define <2 x i1> @setcc_v1i128([1 x i128] %a) {
+; CHECK-LABEL: setcc_v1i128:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    cmpldi r3, 35708
+; CHECK-NEXT:    cmpdi cr1, r4, 0
+; CHECK-NEXT:    li r3, -1
+; CHECK-NEXT:    crnand 4*cr5+lt, 4*cr1+eq, lt
+; CHECK-NEXT:    isel r3, 0, r3, 4*cr5+lt
+; CHECK-NEXT:    mtvsrdd vs34, 0, r3
+; CHECK-NEXT:    blr
+entry:
+  %b = extractvalue [1 x i128] %a, 0
+  %0 = insertelement <2 x i128> undef, i128 %b, i32 0
+  %1 = icmp ult <2 x i128> %0, <i128 35708, i128 1005868>
+  ret <2 x i1> %1
+}