On 07/09/2020 23:21, Luc Van Oostenryck wrote:
>>> diff --git a/validation/optim/and-lsr-or-shl0.c b/validation/optim/and-lsr-or-shl0.c
>>> new file mode 100644
>>> index 000000000000..46ab1bde5249
>>> --- /dev/null
>>> +++ b/validation/optim/and-lsr-or-shl0.c
>>> @@ -0,0 +1,13 @@
>>> +// => 0
>>> +unsigned int and_lsr_or_shl0(unsigned int a, unsigned int b)
>>> +{
>>> + return ((a | b << 12) >> 12) & 0xfff00000;
>>> +}
>>> +
>>> +/*
>>> + * check-name: and-lsr-or-shl0
>>> + * check-command: test-linearize -Wno-decl $file
>>> + * check-known-to-fail
>>> + *
>>> + * check-output-excludes: shl\\.
>>
>> Why not something like:
>> * check-output-contains: ret.32 *\\$0
>
> Yes, at the end this check is the only thing that matters. And since
> it's all pure expressions, if there is a return with a constant,
> no other instructions can possibly be present.
:-D
OK, so let's imagine there is a bug (perhaps an off-by-one, say)
that results in some IR _not_ being removed properly, will this
test catch that? Does it matter? why not?
Given that you don't provide an '*.expected' file, adding the
following to the test won't guarantee to catch that bug either,
but would hopefully increase the odds. (this assumes that the
pre-optimization pass IR contains 'shl', 'or', 'lsr' and 'and',
of course).
>
>> * check-output-excludes: shl\\.
>> * check-output-excludes: or\\.
>> * check-output-excludes: lsr\\.
>> * check-output-excludes: and\\.
>
> But these tests were written (more than 2 years ago, so I forgot the
> details about them) for very specific steps in the simplification
> phase, most probably aiming bitfields (hence the constant shifts &
> masks). In this case it was for a simplification that removed the '<<'.>
[snip]
>>> diff --git a/validation/optim/lsr-or-lsr0.c b/validation/optim/lsr-or-lsr0.c
>>> new file mode 100644
>>> index 000000000000..aad4aa7fda56
>>> --- /dev/null
>>> +++ b/validation/optim/lsr-or-lsr0.c
>>> @@ -0,0 +1,22 @@
>>> +#define S 12
>>> +
>>> +// ((x >> S') | y) >> S;
>>> +// -> ((x >> S' >> S) | (y >> S)
>>
>> s/((x/(x/
>>
>>> +// -> ((x >> 32) | (y >> S)
>>
>> s/((x/(x/
>>
>>> +// => (y >> S)
>>> +
>>> +int lsr_or_lsr0(unsigned int x, unsigned int b)
>>> +{
>>> + return ((x >> (32 - S)) | b) >> S;
>>> +}
>>> +
>>> +/*
>>> + * check-name: lsr-or-lsr0
>>> + * check-command: test-linearize -Wno-decl $file
>>> + * check-known-to-fail
>>> + *
>>> + * check-output-ignore
>>> + * check-output-pattern(1): lsr\\.
>>> + * check-output-excludes: and\\.
>>
>> why would an 'and' be here anyway?
>
> Because each shift has a implicit mask associated with it:
> (x >> S) == ((x & 0xffffffff) >> S) == (x >> S) & 0x000fffff
> In some simplifications I made, it becomes an explicit mask and
> sometimes there was a left-over. But yes, it's not very interesting here.
;-P
So, imagine you didn't know the inner workings of the
optimization code, looking at the test text alone, would
you understand why the test asserts what it does?
(The input IR to the algorithm doesn't have an 'and', right?)
[Similar comment about other email on patch #2, the input IR
doesn't have an 'lsr', right?]
OK, I promise not to comment further on this series! ;-)
ATB,
Ramsay Jones
