This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] [NFC] Add test cases to the ISD::BR_CC node in the instruction selection
ClosedPublic

Authored by HLJ2009 on Nov 21 2018, 9:11 PM.

Details

Summary

Add the following test case for the ISD::BR_CC node in the instruction selection

define i64 @testi64slt(i64 %c1, i64 %c2, i64 %c3, i64 %c4, i64 %a1, i64 %a2) #0 {
entry:
  %cmp1 = icmp eq i64 %c3, %c4
  %cmp3tmp = icmp eq i64 %c1, %c2
  %cmp3 = icmp slt i1 %cmp3tmp, %cmp1
  br i1 %cmp3, label %iftrue, label %iffalse
iftrue:
  ret i64 %a1
iffalse:
  ret i64 %a2
}

The data type i64 can be replaced by i32, i64, float, double
And condition codes can be replaced by: SETEQ, SETEN, SELT, SETLE, SETGT, SETGE,SETULT, SETULE, SSETGT, and SETUGE

Diff Detail

Repository
rL LLVM

Event Timeline

HLJ2009 created this revision.Nov 21 2018, 9:11 PM
steven.zhang accepted this revision.Nov 21 2018, 10:53 PM

You don't need create a review for the new added NFC test case. LGTM.

This revision is now accepted and ready to land.Nov 21 2018, 10:53 PM

Personally, I don't like writing the test case using such script. Because, it will check all the instructions and make your test point unclear. And it hard code the register name, which make this test fragile。

@steven.zhang Thanks for pointing this out, i will consider it.

Personally, I don't like writing the test case using such script. Because, it will check all the instructions and make your test point unclear. And it hard code the register name, which make this test fragile。

This makes maintenance of tests easier since anyone can re-run the script. Of course, the test case is very strictly specified which will make it sensitive to changes in codegen/regalloc/scheduling. However, it is arguably easier for someone to re-run the script and for reviewers to carefully review every aspect of the change than it would be if only specific instructions are checked. There is some value in tests being sensitive to changes not necessarily related to what they are testing - we may discover unintended changes that are actually detrimental.

As to the point about what the test case is actually testing, I think that is a valid concern and a comment should explain that.

steven.zhang requested changes to this revision.Nov 26 2018, 6:09 PM

Personally, I don't like writing the test case using such script. Because, it will check all the instructions and make your test point unclear. And it hard code the register name, which make this test fragile。

This makes maintenance of tests easier since anyone can re-run the script. Of course, the test case is very strictly specified which will make it sensitive to changes in codegen/regalloc/scheduling. However, it is arguably easier for someone to re-run the script and for reviewers to carefully review every aspect of the change than it would be if only specific instructions are checked. There is some value in tests being sensitive to changes not necessarily related to what they are testing - we may discover unintended changes that are actually detrimental.

As to the point about what the test case is actually testing, I think that is a valid concern and a comment should explain that.

Thank you for the detail explanation. So, there are Pro & Cons for both and they are targeting for different purpose. For this case, we shouldn't use this script as we have clear testing point.

This revision now requires changes to proceed.Nov 26 2018, 6:09 PM
HLJ2009 updated this revision to Diff 175381.Nov 26 2018, 7:00 PM
HLJ2009 edited the summary of this revision. (Show Details)
HLJ2009 updated this revision to Diff 175410.Nov 27 2018, 12:43 AM

Using the suggestion to update the test cases. Thanks to Steven and Nemanjai.

steven.zhang accepted this revision.Nov 27 2018, 1:35 AM

LGTM now.

This revision is now accepted and ready to land.Nov 27 2018, 1:35 AM
This revision was automatically updated to reflect the committed changes.