This is an archive of the discontinued LLVM Phabricator instance.

Improve DAGTypeLegalizer::PromoteIntRes_TRUNCATE() to handle widening.
ClosedPublic

Authored by jonpa on Mar 17 2017, 6:10 AM.

Details

Summary

DAGTypeLegalizer::PromoteIntRes_TRUNCATE() did previously not handle the case where the operand needs to be widened, which resultet in a llvm_unreachable()l.

This was discovered with llvm-stress and reported at https://bugs.llvm.org/show_bug.cgi?id=32275. The stress test case is included also in this patch as a regression test for SystemZ.

The test case involves a truncate from v4i8 to v4i1. On SystemZ, v4i8 should be widened to v16i8.

t17: v16i8,ch = CopyFromReg t0, Register:v16i8 %vreg13
  t18: v4i8 = extract_subvector t17, Constant:i32<0>
     t19: v4i1 = truncate t18

The patch net result is (after type legalization)

                 t264: i32 = extract_vector_elt t17, Constant:i32<0>
                  t260: i32 = extract_vector_elt t17, Constant:i32<1>
                  t256: i32 = extract_vector_elt t17, Constant:i32<2>
                  t252: i32 = extract_vector_elt t17, Constant:i32<3>
           t222: v4i32 = BUILD_VECTOR t264, t260, t256, t252
      t112: v4i32 = BUILD_VECTOR Constant:i32<1>, Constant:i32<1>, Constant:i32<1>, Constant:i32<1>
  t224: v4i32 = and t222, t112
t270: v4i32 = sign_extend_inreg t224, ValueType:ch:v4i1

The final output does not look optimal, but since this is so rare, it should not matter.

Diff Detail

Event Timeline

jonpa created this revision.Mar 17 2017, 6:10 AM
RKSimon added a subscriber: RKSimon.
RKSimon added inline comments.
test/CodeGen/SystemZ/vec-trunc-to-i1.ll
4

Reduced with bugpoint:

define void @autogen_SD29574() {
BB:
  %B15 = sub <4 x i8> undef, zeroinitializer
  br label %CF34

CF34:                                             ; preds = %CF34, %BB
  %Tr24 = trunc <4 x i8> %B15 to <4 x i1>
  %Cmp26 = icmp slt <4 x i1> %Tr24, undef
  %E28 = extractelement <4 x i1> %Cmp26, i32 3
  br i1 %E28, label %CF34, label %CF36

CF36:                                             ; preds = %CF34
  ret void
}
jonpa updated this revision to Diff 92151.Mar 17 2017, 9:03 AM

Test case reduced. Thanks Simon.

test/CodeGen/SystemZ/vec-trunc-to-i1.ll
4

Oops - thought I had done that... thanks.

RKSimon added inline comments.Mar 17 2017, 9:09 AM
test/CodeGen/SystemZ/vec-trunc-to-i1.ll
4

You might be able to manually reduce it further. Also, you should probably rename the test pr32275 or something to indicate its origin.

Code changes look fine; testcase could be better.

jonpa updated this revision to Diff 92305.Mar 20 2017, 12:54 AM

Thanks for review.

I changed the name in the test case per Simons suggestion.

I tried also making this even smaller, but found that the problem didn't trigger if I removed the control flow, and my guess is that the DAG optimizations then hide this problem since it's now all just one DAG.

Is this test acceptable, or if not, what is the problem?

I don't like tests which have constant-foldable operation involving undef; you might get lucky and it won't get folded right now, but that could very easily change in the future; then you aren't testing anything useful anymore. In this case, you can reduce the testcase to something like this:

define void @pr32275(<4 x i8> %B15) {
BB:
  br label %CF34

CF34:
  %Tr24 = trunc <4 x i8> %B15 to <4 x i1>
  %E28 = extractelement <4 x i1> %Tr24, i32 3
  br i1 %E28, label %CF34, label %CF36

CF36:
  ret void
}

Also, the CHECK lines should check that the test is actually generating some reasonable sequence, as opposed to just checking that it doesn't crash; that verifies the test is actually testing something relevant.

jonpa updated this revision to Diff 92430.Mar 20 2017, 10:44 PM

In this case, you can reduce the testcase to something like this:

Test updated. CHECK lines generated with update_llc_test_checks.py.

RKSimon accepted this revision.Mar 21 2017, 2:42 AM

LGTM

This revision is now accepted and ready to land.Mar 21 2017, 2:42 AM
jonpa closed this revision.Mar 21 2017, 3:38 AM

Thanks for review, commited as r298357.