With bswap_type being always uint16_type_node for 16bit bswap, I moved the
line to set bswap_type to pass_optimize_bswap::execute() where bswap_type
is set for other sizes.
The following patch was thus committed:
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr64718.c
b/gcc/testsuite/gcc.c-torture/execute/pr64718.c
new file mode 100644
index 0000000..58773e0
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr64718.c
@@ -0,0 +1,18 @@
+static int __attribute__ ((noinline, noclone))
+swap (int x)
+{
+ return (unsigned short) ((unsigned short) x << 8 | (unsigned short) x >> 8);
+}
+
+static int a = 0x1234;
+
+int
+main (void)
+{
+ int b = 0x1234;
+ if (swap (a) != 0x3412)
+ __builtin_abort ();
+ if (swap (b) != 0x3412)
+ __builtin_abort ();
+ return 0;
+}
diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
index 479f49f..e30116d 100644
--- a/gcc/tree-ssa-math-opts.c
+++ b/gcc/tree-ssa-math-opts.c
@@ -2355,30 +2355,28 @@ bswap_replace (gimple cur_stmt, gimple src_stmt, tree
fndecl, tree bswap_type,
tmp = src;
+ /* Convert the src expression if necessary. */
+ if (!useless_type_conversion_p (TREE_TYPE (tmp), bswap_type))
+ {
+ gimple convert_stmt;
+
+ tmp = make_temp_ssa_name (bswap_type, NULL, "bswapsrc");
+ convert_stmt = gimple_build_assign (tmp, NOP_EXPR, src);
+ gsi_insert_before (&gsi, convert_stmt, GSI_SAME_STMT);
+ }
+
/* Canonical form for 16 bit bswap is a rotate expression. Only 16bit values
are considered as rotation of 2N bit values by N bits is generally not
- equivalent to a bswap. Consider for instance 0x01020304 >> 16 which gives
- 0x03040102 while a bswap for that value is 0x04030201. */
+ equivalent to a bswap. Consider for instance 0x01020304 r>> 16 which
+ gives 0x03040102 while a bswap for that value is 0x04030201. */
if (bswap && n->range == 16)
{
tree count = build_int_cst (NULL, BITS_PER_UNIT);
- bswap_type = TREE_TYPE (src);
- src = fold_build2 (LROTATE_EXPR, bswap_type, src, count);
+ src = fold_build2 (LROTATE_EXPR, bswap_type, tmp, count);
bswap_stmt = gimple_build_assign (NULL, src);
}
else
- {
- /* Convert the src expression if necessary. */
- if (!useless_type_conversion_p (TREE_TYPE (tmp), bswap_type))
- {
- gimple convert_stmt;
- tmp = make_temp_ssa_name (bswap_type, NULL, "bswapsrc");
- convert_stmt = gimple_build_assign (tmp, NOP_EXPR, src);
- gsi_insert_before (&gsi, convert_stmt, GSI_SAME_STMT);
- }
-
- bswap_stmt = gimple_build_call (fndecl, 1, tmp);
- }
+ bswap_stmt = gimple_build_call (fndecl, 1, tmp);
tmp = tgt;
@@ -2386,6 +2384,7 @@ bswap_replace (gimple cur_stmt, gimple src_stmt, tree
fndecl, tree bswap_type,
if (!useless_type_conversion_p (TREE_TYPE (tgt), bswap_type))
{
gimple convert_stmt;
+
tmp = make_temp_ssa_name (bswap_type, NULL, "bswapdst");
convert_stmt = gimple_build_assign (tgt, NOP_EXPR, tmp);
gsi_insert_after (&gsi, convert_stmt, GSI_SAME_STMT);
@@ -2498,7 +2497,7 @@ pass_optimize_bswap::execute (function *fun)
/* Already in canonical form, nothing to do. */
if (code == LROTATE_EXPR || code == RROTATE_EXPR)
continue;
- load_type = uint16_type_node;
+ load_type = bswap_type = uint16_type_node;
break;
case 32:
load_type = uint32_type_node;
Best regards,
Thomas
> -----Original Message-----
> From: Richard Biener [mailto:rguent...@suse.de]
> Sent: Friday, January 23, 2015 6:19 PM
> To: Thomas Preud'homme
> Cc: gcc-patches@gcc.gnu.org
> Subject: RE: [PATCH] Fix PR64718: bad 16-bit bswap replacement
>
> On Fri, 23 Jan 2015, Thomas Preud'homme wrote:
>
> > > From: Richard Biener [mailto:rguent...@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.
