[llvm] r303133 - [InstCombine] add tests for PR32791; NFC

Davide Italiano via llvm-commits llvm-commits at lists.llvm.org
Tue May 16 10:04:20 PDT 2017


On Tue, May 16, 2017 at 9:13 AM, Sanjay Patel <spatel at rotateright.com> wrote:
>
> On Mon, May 15, 2017 at 8:03 PM, Davide Italiano <davide at freebsd.org> wrote:
>>
>> On Mon, May 15, 2017 at 6:35 PM, Sanjay Patel <spatel at rotateright.com>
>> wrote:
>> > This isn't "code"; it's test coverage. In any case, I won't be able
>> > revert
>> > this myself until tomorrow morning (mountain time zone). Since you
>> > obviously
>> > object to this commit, can you revert it for me?
>> >
>> > note that in addition to a potential transform, these tests add coverage
>> > for
>> > a potential infinite loop from conflicting InstCombines. Having
>> > encountered
>> > more than my fair share of those, I'd like to see far more of these
>> > negative
>> > regression tests, so I hit them during development rather than via bug
>> > report.
>> >
>>
>> There's no rush reverting this, as it's not breaking things.
>> I'm fine with leaving the tests for the infinite loop, but please add
>> a comment explaining that.
>
>
>
> Patch posted for review:
> https://reviews.llvm.org/D33242
>
>
>>
>> I'd appreciate if we can continue the discussion on whether this
>> transformation should live on phabricator before we go forward with
>> this.
>
>
> Sure - it wasn't my intention to imply that we are adding InstCombine folds
> based on these tests, but I can see how that might be assumed. I'll add more
> commentary with similar commits. In many cases like this, I have tried to
> improve the LLVM regression test directory cultural norms by
> using/requesting meaningful names for files and tests and/or adding
> comments, but recent commits show that "foo" and "poo" without any
> explanation are still accepted.
>
>

Most of the tests are still reduced with `opt -metarenamer` which
gives funny name, at times.
Some of us have their favourite lucky variable name, and use in the tests.
The name of the functions and the variables used in the test don't
matter, IMHO. What matters is:
1) Proper comments
2) If derived from a PR, name the file PRxxxxxxx.
3) Location in the tree.

-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare


More information about the llvm-commits mailing list