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

Qing Shan Zhang via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 26 18:10:10 PST 2018


steven.zhang requested changes to this revision.
steven.zhang added a comment.
This revision now requires changes to proceed.

In D54824#1308344 <https://reviews.llvm.org/D54824#1308344>, @nemanjai wrote:

> In D54824#1306039 <https://reviews.llvm.org/D54824#1306039>, @steven.zhang wrote:
>
> > 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54824/new/

https://reviews.llvm.org/D54824





More information about the llvm-commits mailing list