[PATCH] D6995: Reassociate: reprocess RedoInsts after each instruction

Mon Nov 16 15:19:24 PST 2015

> On 2015-Nov-16, at 15:00, Quentin Colombet <qcolombet at apple.com> wrote:
> 
>> 
>> On Nov 16, 2015, at 2:47 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:
>> 
>>> 
>>> On Nov 16, 2015, at 2:36 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>> 
>>> 
>>>> On Nov 16, 2015, at 2:24 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:
>>>> 
>>>>> 
>>>>> On Nov 16, 2015, at 2:09 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>>> 
>>>>>> 
>>>>>> On 2015-Nov-16, at 13:54, Mehdi Amini <mehdi.amini at apple.com> wrote:
>>>>>> 
>>>>>> 
>>>>>>> On Nov 16, 2015, at 1:50 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>> On 2015-Nov-16, at 13:47, Mehdi AMINI <mehdi.amini at apple.com> wrote:
>>>>>>>> 
>>>>>>>> joker.eph added a comment.
>>>>>>>> 
>>>>>>>> Only 3 test are still crashing now, but at the time I wrote the patch, I iterated on a fuzzer and these six tests (not eight) were stressing different patterns and different part of reassociate, so they're absolutely not redundant.
>>>>>>> 
>>>>>>> You could use six different functions in a single .ll file, right?
>>>>>> 
>>>>>> I “could"
>>>>> 
>>>>> I mean, that gives the same coverage, right?  Since it seems to reduce
>>>>> the ongoing cost of running
>>>> 
>>>> Cost of running? You’re launching less processes but you’re losing parallelism. I guess we have enough of the latter, but are we really limited by the first right now?
>>>> 
>>>>> and maintaining the test(s), then merging
>>>>> them seems better.
>>>> 
>>>> How is the cost of maintenance reduced? If only one case is crashing, I found it more convenient to investigate and maintain if they’re in separate files.
>>> 
>>> If they are different crashes, I expect to have different commit/fix for each of them.
>> 
>> Fair enough, the revision history shows that I improved the patch with 4 different diffs (and the tests were added as I found them), I could split the revision in 4 again and come up with separate patched to fix each of the individual tests incrementally. 
>> But I don’t think it would be the best use of my time (or the reviewers' time) at this point.
> 
> That’s up to you. Having the fixes reviewed together and the commit being separated is also possible.
> 
>> 
>>> 
>>>> 
>>>>> 
>>>>>>>> I'm not sure either what CHECK line to put for a compiler crash non-regression, there are multiple cases in the test suite like this where FileCheck is not involved.
>>>>>>> 
>>>>>>> It's best practice to check for an expected output even if it's just
>>>>>>> a crasher.  Most of the tests where FileCheck isn't involved are
>>>>>>> legacy (and should be fixed if possible).
>>>>>> 
>>>>>> Should I just add a CHECK-LABEL with the function name? 
>>>>>> What is the added value for the test?
>>>>> 
>>>>> Usually you can find *something* that adds value.  For this, I imagine
>>>>> you could check that -reassociate transforms the IR the way you
>>>>> expect…
>>>> 
>>>> I consider this bad practice: this increases the cost of maintenance without adding any value.
>>> 
>>> Well, it makes sure the IR output is somehow correct.
>>> Where is no point in “not crashing” if that means generating invalid IR, right?
>> 
>> There are other tests for this purpose.
>> Or on the extreme we could just dump the input IR of the benchmark suite and add check on every line of the output of  "opt -O3”.
> 
> That’s not realistic because O3 involves a lot of moving pieces.
> The point is to make sure *this* specific optimization does the proper thing in *these* cases. Obviously, this is not covered by other tests, since we just found the crashes now.
> The bottom line is I do believe there is value in adding check lines in those tests.
> 
>> 
>>> 
>>>> I don’t actually care what reassociate produce on these examples, as long as it does not crash. I could just test that the output does not change compared to what is produced right now. But it increases the maintenance cost because whenever reassociate becomes smarter or find a new canonicalization, this test may need to be audited and updated. I can’t imagine this to scale.
>>> 
>>> I disagree, it helps to see how reassociate becomes smarter when there is a commit enhancing it.
>>> Yes, this is more work for someone creating the patch, but I think it is necessary work.
>> 
>> No, my experience tells me this is really bad. 
>> To have worked on such a codebase (not that long ago), it is very quickly a maintenance nightmare because any of your patch that change a canonicalization will “break” tens of tests and you spent more time trying to understand if you broke these tests or if these tests were just not testing the right thing the right way.
> 
> Sure, changing the canonicalization will break a lot of tests, but I do not see why this is bad. I agree this will mean a lot of work for the one patching the files, but frankly, maybe this is me not working enough on the IR passes, but I’ve never seen that being a problem.
> The take away is I’d rather spend some time updating tests than missing invalid IR or missed canonicalization to keep your example. 
> 
>> Any check I would add here would not mean the IR is in “canonical” or desired form, and I don’t see how it could be considered better to have such a check rather than none.
> 
> Again you are exercising IR that was never seen before and it is important we check the code is correct IMHO.
> 
> Anyway, again these are my 2 cents. Other people have looked into your patch and gave their opinion on the tests. So if they are okay, that’s fine with me. It just means we disagree on the best practices for writing tests, but this is fine, I don’t pretend to know the truth :).
> 
> Cheers,
> Q.

Quentin's suggestion follows the commonly accepted practice in LLVM as
I understand it, and I agree that it makes good sense here.

I've updated many many IR testcases.  IMO, the key to understanding
whether you're updating a test correctly is the comment at the top of
the test file, ideally that includes a PR (or an internal bug tracker,
but then outside people need to ask it to be looked up (so the comment
is the most important)).

Adding David Blaikie for his opinion, since he's updating tons of IR
tests as well for his opaque pointer type work.