Re: [patch, tree-ssa] PR54295 Incorrect value extension in widening multiply-accumulate

[Richard Earnshaw <rearnsha at arm dot com>]

On 20/08/12 12:36, Richard Guenther wrote:
> On Fri, Aug 17, 2012 at 7:05 PM, Richard Earnshaw <rearnsha@arm.com> wrote:
>> On 17/08/12 16:20, Richard Earnshaw wrote:
>>> Ok, in which case we have to give is_widening_mult_rhs_p enough smarts
>>> to not strip
>>>
>>>       (s32)u32
>>>
>>> and return u32.
>>>
>>> I'll have another think about it.
>>
>> Take two.
>>
>> This version should address your concerns about handling
>>
>>         (u32)u16 * (u32)u16 -> u64
>>
>> We now look at each operand directly, but when doing so we check whether
>> the operand is the same size as the result or not.  When it is, we can
>> strip any conversion; when it isn't the conversion must preserve
>> signedness of the inner operand and mustn't be a narrowing conversion.
>>
>>         * tree-ssa-math-opts.c (widening_mult_conversion_strippable_p):
>>         New function.
>>         (is_widening_mult_rhs_p): Use it.
>>
>> Testing underway (again)
>>
>> OK?
> 
> Ok.
> 
> Thanks,
> Richard.
> 
>> R.
>>
> 


It turns out that this patch caused a few tests in gcc.target/arm to
start failing.  Investigating has shown that there are a number of
reasons for this.

One test (gcc.target/arm/pr50318-1.c) is just bogus.  It's scanning for
an unsigned widening multiply when the correct operation is a signed
widening multiply.  Fixing the compiler has just exposed the broken test.

Two tests (gcc.target/arm/smlaltb-1.c and gcc.target/arm/smlaltt-1.c)
fail because the compiler now generates a subreg in the middle of the
widening multiply expression.  Our patterns don't recognize this form
and I'm really not keen on the compiler doing this sort of thing anyway,
subreg operations of this nature are endian dependent and dealing with
this is messy.  For now I'm going to xfail these two tests.

The final two failures (gcc.target/arm/wmul-5.c and
gcc.target/arm/wmul-6.c) really should pass.  They don't because my
first iteration of the patch failed to spot that (sign_extend:DI
(zero_extend:SI (reg:HI))) can be simplified legitimately to
(zero_extend:DI (reg:HI)).  The patch below to
widening_mult_conversion_strippable_p fixes this.

So this is a clean-up patch to fix these issues.

	* tree-ssa-math-opts.c (widening_mult_conversion_strippable_p):
	Sign-extension of a zero-extended value can be simplified to
	just zero-extension.

testsuite:

	* gcc.target/arm/pr50318-1.c: Scan for smlal.
	* gcc.target/arm/smlaltb-1.c: XFAIL test.
	* gcc.target/arm/smlaltt-1.c: Likewise.

OK?

Attachment: mul-extend2.patches
Description: Text document