Page MenuHomePhabricator

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

Authored by ZhangKang on Jul 21 2020, 6:51 AM.

Details

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.

Diff Detail

Event Timeline

ZhangKang created this revision.Jul 21 2020, 6:51 AM
lkail added a subscriber: lkail.Jul 21 2020, 7:58 AM
lkail added inline comments.
llvm/test/CodeGen/PowerPC/setcc-vector.ll
5

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

ZhangKang updated this revision to Diff 279697.Jul 21 2020, 7:30 PM

Add more test cases.

ZhangKang marked an inline comment as done.Jul 21 2020, 7:30 PM

Your test didn't crash even without your fix.

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 added inline comments.Jul 21 2020, 10:01 PM
llvm/test/CodeGen/PowerPC/setcc-vector.ll
7

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

Remove le for PWR7.

ZhangKang marked an inline comment as done.Jul 21 2020, 10:46 PM

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 ?

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
}

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 updated this revision to Diff 279854.Jul 22 2020, 9:38 AM

Update the test case.

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 updated this revision to Diff 280012.Jul 22 2020, 8:08 PM

Remove the hasP8Vector condition.

steven.zhang accepted this revision.Jul 28 2020, 7:47 PM

LGTM now. Thank you for fixing this.

This revision is now accepted and ready to land.Jul 28 2020, 7:47 PM
This revision was landed with ongoing or failed builds.Jul 29 2020, 9:40 AM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Tue, Sep 1, 8:21 AM

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.