[llvm] r186119 - In response to dblaikie's comment on r186035, replacing the

[David Blaikie]

On Fri, Jul 12, 2013 at 2:05 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
> On Jul 12, 2013, at 1:54 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Thu, Jul 11, 2013 at 2:16 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>> Author: adrian
>>> Date: Thu Jul 11 16:16:14 2013
>>> New Revision: 186119
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=186119&view=rev
>>> Log:
>>> In response to dblaikie's comment on r186035, replacing the
>>> (reduced LLVM IR) + (full source in comment)
>>> with the
>>> (full LLVM IR) + (reduced src in comment)
>>
>> Thanks
>>
>> Though I'm still curious if this is the simplest example - could you
>> help me understand what the bug was & what this example is attempting
>> to demonstrate?
>
> This testcase only exists because Eric specifically requested one for this commit:
> http://llvm.org/viewvc/llvm-project?rev=186014&view=rev

Sure - because all changes should be tested, but that alone doesn't
quite explain to me what it's trying to test (the last comment you
made about the InlineSpiller helps - but this test case still looks a
bit big for that. I assume the real need was to have a function call
inside a function that took a non-trivial by-value parameter, the
function calling trying to spill)

I doubt the bug has anything to do with operator overloading or
inheritance, pointers, etc... so there still seems to be a lot of
noise in this test case.

The reason I want test cases as reduced as possible is that if we have
test cases that are incidentally testing a lot of other things it's
more costly when we make changes - we don't want to have to visit all
the test cases all the time.

>> Indirect parameters were an issue I fixed when I added the
>> "isIndirect" flag, with the simple test case in
>> test/DebugInfo/X86/arguments.ll
>>
>> What is it that was still broken/that you fixed? (it might even make
>> sense to just expand the arguments.ll test case to include an example
>> that triggers your bug)
>
> See below the discussion over in the main thread for this:
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130708/180776.html
>>
>> Also - tests that are simply "test we don't crash" are pretty low
>> value - presumably rather than crashing there's some /correct/
>> behavior we should have not just "do anything other than crash". We
>> should be testing for whatever that correct/desired behavior is (the
>> critical piece we were missing/that lead to the bug, anyway).
>
> If you look at the commit history, this commit this testcase belongs to fixes a assertion/crash. That’s why the testcase tests that we don’t crash.

Yet nothing tests that the behavior for this case, the one where we
previously crashed, does anything useful - it's not possible that any
other test case is testing this, because any such test case
wouldn't've been passing previously.

Crashes demonstrate that we have a test coverage gap. Testing that we
don't crash is not sufficient - there is some behavior we expect out
of the compiler in this case & we have no test case to verify that we
get that behavior. All we have is a test that verifies that we don't
crash - we could be doing anything else (/in this case/).

So I went to reduce this test case myself, but the first step was to
check that the test case demonstrates the bug that was fixed. It
doesn't seem to - I attempted to revert your fix (r185966 - had some
merge conflicts) & rerun the test - it still passes. Is there
something else you changed that this test case demonstrates the fix
of?

In any case, once I can reproduce the failure I can further simplify
(& constrain, by testing the output rather than just the absence of a
crash) the test to demonstrate what I'm talking about/asking for.

> If you are interested in a test case for the desired behavior, have a look at
> http://llvm.org/viewvc/llvm-project?rev=185966&view=rev
> there’s plenty.
>
> -- adrian