On Tue, Jul 12, 2011 at 1:04 PM, Richard Guenther
<richard.guent...@gmail.com> wrote:
> On Tue, Jul 12, 2011 at 11:50 AM, Andrew Stubbs <a...@codesourcery.com> wrote:
>> On 23/06/11 15:39, Andrew Stubbs wrote:
>>>
>>> This patch has two effects:
>>>
>>> 1. It permits the use of widening multiply instructions that widen by
>>> more than one mode. E.g. HImode -> DImode.
>>>
>>> 2. It enables the use of widening multiply instructions for (extended)
>>> inputs of narrower mode than the instruction takes. E.g. QImode ->
>>> DImode where only HI->DI or SI->DI is available.
>>>
>>> Hopefully, most of the patch is self-explanatory, but here are few notes:
>>>
>>> The code introduces a temporary FIXME comment; this will be removed
>>> later in the patch series. In fact, this is not a new restriction;
>>> previously "type1" and "type2" were implicitly identical because they
>>> were required to be one mode smaller than "type".
>>>
>>> I regard the ARM portion of this patch as obvious, so I don't think I
>>> need an ARM maintainer to read this.
>>>
>>> Is the patch OK?
>>
>> I found a bug in this patch. It seems I do need to add casts for the inputs
>> to widening multiplies (even though I know the registers are already fine),
>> because otherwise something is insisting on truncating the values to the
>> minimum width, which isn't helpful when it's actually an instruction with
>> wider inputs.
>>
>> The mode changing bits from patch 4 have therefore been moved here. I've
>> made the changes Richard Guenther requested there, I think.
>>
>> Otherwise, the patch is the same as before.
>
> I wonder if we want to restrict the WIDEN_* operations to operate
> on types that have matching type/mode precision(**). Consider
>
> struct {
> int a : 7;
> int b : 7;
> } x;
>
> short c = x.a * x.b;
>
> which will be represented as (short)((int)<7-bit-type-with-QImode> *
> (int)<7-bit-type-with-QImode>).
>
> I wonder if you can do some experiments with bitfield types and see
> if your patch series handles them correctly.
>
> As for the patch, please update tree.def with the new requirements
> for the WIDEN_* codes.
>
> As for the bitfield precisions, we probably want to reject types that
> do not have TYPE_PRECISION (type) == GET_MODE_PRECISION (TYPE_MODE
> (type)). Or maybe we can allow them if we generate
> correct and good code for them?
>
> + tmp2 = TYPE_UNSIGNED (type1) == TYPE_UNSIGNED (type2)
> + ? tmp1
> + : create_tmp_var (
> + build_nonstandard_integer_type (
> + GET_MODE_PRECISION (from_mode), TYPE_UNSIGNED (type1)),
> + NULL);
>
> please use an if () stmt to avoid gross formatting.
>
> + if (TYPE_MODE (type1) != from_mode)
>
> these kind of checks are unsafe if type1 does not have a TYPE_PRECISION
> equal to its mode precision.
(**) We really ought to forbid any arithmetic on types that have non-mode
precision and only allow conversions to/from such types.
