[llvm-dev] [PATCH] D50328: [X86][SSE] Combine (some) target shuffles with multiple uses

[David Greene via llvm-dev]

Simon Pilgrim <llvm-dev at redking.me.uk> writes:

> Changing a test's IR to avoid an issue in a patch is very problematic,
> but if any test's codegen changes because of a patch then it just
> needs to be reviewed, preferably by someone who has touched that test
> in the past.

But wouldn't it be even better if that output didn't need to be changed
at all and therefore didn't need to be reviewed?  Right now a lot of X86
changes have a lot of noise in the diff due to test output (asm)
changes.  A fair majority of those changes are incidental to a given
patch.

>> How does the script speed up creation of new tests?
> By automating the creation of the CHECKs, allowing the developer to
> focus on writing/reducing the IR. There is no way we could have added
> the exhaustive tests for much of the custom lowering (vector and
> scalar codegen) across so many x86 subtargets without the scripts to
> do the checks, it would have taken far too long - people would simply
> not spend the time to add as much coverage and we would end up missing
> things.

Ok, that sounds interesting.  The comment at the top of the script
implies that it takes existing tests with CHECK lines and updates them.
Is it also somehow able to *create* CHECK lines from raw asm output?
I'm just trying to understand the tool.

Does the script simply pull in the latest asm and re-emit CHECK lines
with the raw asm?  If so, I can see why editing tests to remove
hard-coded registers might be scary.  It would mean someone would have
to go back in and change all the hard-coded names back to variables
every time the script is run.  Is there some way we can improve the
script to do this automatically?

Auto-generating stuff is nice.  But I don't think we should say that it
replaces work to make small, focused and robust tests.  Testing is hard.

>> What kinds of dodgy code being hidden are you concerned about?  Can you
>> provide an example?
> It happens in many ways, for example: often in the intro/exit code
> that isn't considered directly relevant; or it checks that exactly one
> instance of an instruction occurs but doesn't consider that a similar
> instruction might be there as well (common with shuffles); or failure
> to completely show how 'pre-committed' tests are changed by a
> patch. The update script helps ensure we have a whole view of the
> codegen to avoid these, and then in review more eyes can check it.

I guess I don't see how hard-coding register names helps with those
kinds of things.  Ideally "pre-committed" code would not change many
tests at all.

If someone is worried about prolog/epilog code then there should be
tests specifically targeted to that.  Not every test needs to include
CHECK lines for that stuff.

I am not suggesting tests need to be perfect, just that we should try to
make them less brittle as we go along, incrementally.

>> If a test is looking for different behavior based on register use, then
>> of course the test should be written to detect specific register uses.
>> If the register use changes, the test fails because a bug was
>> introduced.
>>
>> I would bet 95% of X86 tests don't care what registers are used
>> whatsoever.  From my perspective, it would be much better to have 95% of
>> tests never change over time, leaving a much smaller set of tests that
>> may never (ideally will not ever) need to be changed, because they are
>> written to expect specific sequences and violations of those are bugs.
> True - many test cases don't care about the exact register used. But
> register/codegen changes indicates a change in behaviour that can be
> investigated and help ensure the patch is doing what you want.

But why should everyone review lots of lines that just don't matter?  I
find that when I look at X86 changes I tend to gloss over test changes
because there are just so many of them and I am not at all familiar with
what the tests are actually testing.  We all have limited time.  I could
argue that the excessive noise in diffs actually makes it more likely
that problems creep in.

>> Tests should be focused.  If we're using, for example, a bunch of tests
>> develped for shuffle selection as proxies for register allocation,
>> that's not good.  We should have specific register allocation tests.
> I don't know of any examples where we are doing anything so brittle
> (although many bug tests could be testing for that "perfect storm",
> you can only reduce so far) - we have instructions that can only use
> certain registers (e.g. DIV/MUL, PBLENDV's implicit use of xmm0 or
> EVEX's upper 15 zmm registers) and ensuring that we handle that
> efficiently can be more than a regalloc only issue.

Isn't the very change under discussion an example of this?  Lots of
tests have changed hunks that needn't be there.  That's brittle.

ISA register assignment requirements should, again, have tests
specifically to ensure we don't break it.  I assume we already have many
of them.

>> Is this unreasonable?  Can we not incrementally improve the tests?
> Ensuring tests are focussed and continue to test what they are
> supposed to is never unreasonable.

Good!  :)

Perhaps we can improve the tool to make maintenance easier.

                                  -David