[PATCH] D6995: Reassociate: reprocess RedoInsts after each instruction
Mehdi Amini via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 16 14:24:36 PST 2015
> 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.
>
>>>> 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.
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.
—
Mehdi
More information about the llvm-commits
mailing list