On 02/04/10 08:39, Rahul Kharche wrote:
Hi All,
On our private port of GCC 4.4.1 we fail to combine successive SHIFT
operations like in the following case
#include<stdlib.h>
#include<stdio.h>
void f1 ()
{
unsigned short t1;
unsigned short t2;
t1 = rand();
t2 = rand();
t1<<= 1; t2<<= 1;
t1<<= 1; t2<<= 1;
t1<<= 1; t2<<= 1;
t1<<= 1; t2<<= 1;
t1<<= 1; t2<<= 1;
t1<<= 1; t2<<= 1;
printf("%d\n", (t1+t2));
}
This is a ZERO_EXTEND problem, because combining SHIFTs with whole
integers works correctly, so do signed values. The problem seems to
arise in the RTL combiner which combines the ZERO_EXTEND with the
SHIFT to generate a SHIFT and an AND. Our architecture does not
support AND with large constants and hence do not have a matching
insn pattern (we prefer not doing this, because of large constants
remain hanging at the end of all RTL optimisations and cause needless
reloads).
Fixing the combiner to convert masking AND operations to ZERO_EXTRACT
fixes this issue without any obvious regressions. I'm adding the
patch here against GCC 4.4.1 for any comments and/or suggestions.
Good catch. However, note we are a regression bugfix only phase of
development right now in preparation for branching for GCC 4.5. As a
result the patch can't be checked in at this time; I would recommend you
update the patch to the current sources & attach it to bug #41998 which
contains queued patches for after GCC 4.5 branches.
Cheers,
Rahul
--- combine.c 2009-04-01 21:47:37.000000000 +0100
+++ combine.c 2010-02-04 15:04:41.000000000 +0000
@@ -446,6 +446,7 @@
static void record_truncated_values (rtx *, void *);
static bool reg_truncated_to_mode (enum machine_mode, const_rtx);
static rtx gen_lowpart_or_truncate (enum machine_mode, rtx);
+static bool can_zero_extract_p (rtx, rtx, enum machine_mode);
/* It is not safe to use ordinary gen_lowpart in combine.
@@ -6973,6 +6974,16 @@
make_compound_operation (XEXP (x, 0),
next_code),
i, NULL_RTX, 1, 1, 0, 1);
+ else if (can_zero_extract_p (XEXP (x, 0), XEXP (x, 1), mode))
+ {
+ unsigned HOST_WIDE_INT len = HOST_BITS_PER_WIDE_INT
+ - CLZ_HWI (UINTVAL (XEXP (x,
1)));
+ new_rtx = make_extraction (mode,
+ make_compound_operation (XEXP (x, 0),
+ next_code),
+ 0, NULL_RTX, len, 1, 0,
+ in_code == COMPARE);
There should be a comment prior to this code fragment describing the
transformation being performed. Something like:
/* Convert (and (shift X Y) MASK) into ... when ... */
That will make it clear in the future when your transformation applies
rather than forcing someone scanning the code to read it in detail.
+ }
break;
@@ -7245,6 +7256,25 @@
return simplify_gen_unary (TRUNCATE, mode, x, GET_MODE (x));
}
+static bool
+can_zero_extract_p (rtx x, rtx mask_rtx, enum machine_mode mode)
There should be a comment prior to this function which briefly describes
what the function does, the parameters & return value. Use comments
prior to other functions to guide you.
@@ -8957,7 +8987,6 @@
op0 = UNKNOWN;
*pop0 = op0;
-
/* ??? Slightly redundant with the above mask, but not entirely.
Moving this above means we'd have to sign-extend the mode mask
for the final test. */
Useless diff fragment. Remove this change as it's completely unrelated
and useless.
You should also write a ChangeLog entry for your patch. ChangeLogs
describe what changed, not why something changed. So a suitable entry
might look something like:
<date> Your Name <yourem...@somewhere>
* combine.c (make_compound_operation): Convert shifts and masks
into zero_extract in certain cases.
(can_zero_extract_p): New function.
If you could make those changes and attach the result to PR 41998 they
should be able to go in once 4.5 branches from the mainline.
Jeff
