RE: [PATCH] Fix PR64718: bad 16-bit bswap replacement

[Richard Biener <rguenther at suse dot de>]

On Fri, 23 Jan 2015, Thomas Preud'homme wrote:

> > From: Richard Biener [mailto:rguenther@suse.de]
> > Sent: Friday, January 23, 2015 6:01 PM
> > > +  if (bswap && n->range == 16)
> > > +    bswap_type = TYPE_UNSIGNED (TREE_TYPE (src)) ?
> > short_unsigned_type_node
> > > +						 :
> > short_integer_type_node;
> > 
> > I don't think using short_unsigned_type_node or
> > short_integer_type_node
> > is correct - that depends on the SHORT_TYPE_SIZE target macro which
> > may very well not match HImode.
> 
> Indeed, my mistake. What about intHI_type_node and unsigned_intHI_type_node?
> 
> > 
> > I think for the rotation itself whether the type is signed or unsigned
> > doesn't matter and the bswap builtins expect unsigned input.  So I think
> > you should use an unsigned type unconditionally.
> 
> Ok then only unsigned_intHI_type_node.

That should work - you already use uint16_type_node for load_type
so I suggest to use uint16_type_node instead of unsigned_intHI_type_node.
That's technically even more correct if you consider BITS_PER_UNIT != 8
(though bswap pass is disabled in that case).

> > 
> > Which means you can simply use build_nonstandard_integer_type (16,
> > 1);
> > or even bswap_type as-is (it should match the unsigned builtin argument
> > type already?)
> 
> bswap_type is not set for 16-bit bswap since we removed the need to have
> a builtin bswap in the first place, hence this line.

I see.

Ok with using uint16_type_node.

Thanks,
Richard.