This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ] Enable trackSubRegLiveness
AbandonedPublic

Authored by kparzysz on Aug 13 2018, 10:30 AM.

Details

Reviewers
jonpa
uweigand
Summary

Fix a couple of bugs demonstrated in llvm.org/PR38544.

Diff Detail

Repository
rL LLVM

Event Timeline

kparzysz created this revision.Aug 13 2018, 10:30 AM
jonpa added a comment.Aug 14 2018, 1:36 AM

SystemZ test updates at https://reviews.llvm.org/D50690 (code changes seem fine).

I will do more testing and update any more SystemZ tests as needed, but I don't want to try to review the register allocator patches since I am not that familiar with it.

jonpa added a comment.Aug 14 2018, 2:37 AM

I have found more crashes with randomized testing over last night, which also persist with your latest fixes.

I found three types of failures and posted three reduced tests with run lines on the Bugzilla post.

kparzysz updated this revision to Diff 160633.Aug 14 2018, 10:25 AM

Fixes for remaining testcases, except tc_subregliveness_noliveseg.ll. That one requires D50725.

jonpa added a comment.Aug 15 2018, 6:15 AM

Thanks again for working on this :-)

With your latest fixes applied (including the subregindex patch), I could see just one more test failure from last night, which I uploaded on Bugzilla.

Maybe it would be a good idea to have an option in the SystemZ enableSubRegLiveness() method that enables subreg liveness which (at least initially) defaults to off, so that you can commit your fixes with test cases for SystemZ? Perhaps some word of caution like "experimental" could be used somewhere...?

I don't think FileCheck is needed to check for successful compilation. For instance test/CodeGen/SystemZ/dag-combine-04.ll just runs llc and the comment explains what is supposed not to crash.

The tc_... naming are probably not ideal for SystemZ tests, sorry. subregliveness-01.ll, subregliveness-02.ll, ... are probably better names.

The option would be great. Then I could commit the fixes individually with the corresponding testcases.

jonpa added a comment.Aug 15 2018, 7:03 AM

The option would be great. Then I could commit the fixes individually with the corresponding testcases.

Patch on the way with https://reviews.llvm.org/D50779.

kparzysz abandoned this revision.Aug 23 2018, 9:14 AM

The fixes have been committed individually.