On 13/05/16 14:01, Bernd Schmidt wrote:
On 05/13/2016 02:21 PM, Kyrill Tkachov wrote:
Hi all,
In this PR we may end up shifting a negative value left in
simplify_comparison.
The statement is:
const_op <<= INTVAL (XEXP (op0, 1));
This patch guards the block of that statement by const_op >= 0.
I _think_ that's a correct thing to do for that trasnformation:
"If we have (compare (xshiftrt FOO N) (const_int C)) and
the low order N bits of FOO are known to be zero, we can do this
by comparing FOO with C shifted left N bits so long as no
overflow occurs."
Isn't the condition testing for overflow though? In a somewhat nonobvious way.
So I think you should just use an unsigned version of const_op. While we're
here we might as well make the code here a little more readable. How about the
patch below? Can you test whether this works for you?
Looks reasonable to me barring some comments below.
I'll test a version of the patch with the comments fixed.
Thanks,
Kyrill
Bernd
Index: combine.c
===================================================================
--- combine.c (revision 236113)
+++ combine.c (working copy)
@@ -11628,13 +11628,13 @@ simplify_comparison (enum rtx_code code,
while (CONST_INT_P (op1))
{
+ HOST_WIDE_INT amount;
This has to be an rtx rather than HOST_WIDE_INT from the way you use it later
on.
/* We only want to handle integral modes. This catches VOIDmode,
CCmode, and the floating-point modes. An exception is that we
@@ -11649,7 +11649,8 @@ simplify_comparison (enum rtx_code code,
/* Try to simplify the compare to constant, possibly changing the
comparison op, and/or changing op1 to zero. */
code = simplify_compare_const (code, mode, op0, &op1);
- const_op = INTVAL (op1);
+ HOST_WIDE_INT const_op = INTVAL (op1);
+ unsigned HOST_WIDE_INT uconst_op = (unsigned HOST_WIDE_INT) const_op;
Can this be just "unsigned HOST_WIDE_INT uconst_op = UINTVAL (op1);" ?
...
+ unsigned HOST_WIDE_INT low_mask
+ = (((unsigned HOST_WIDE_INT) 1 << INTVAL (amount)) - 1);
unsigned HOST_WIDE_INT low_bits
- = (nonzero_bits (XEXP (op0, 0), mode)
- & (((unsigned HOST_WIDE_INT) 1
- << INTVAL (XEXP (op0, 1))) - 1));
+ = (nonzero_bits (XEXP (op0, 0), mode) & low_mask);
if (low_bits == 0 || !equality_comparison_p)
{
(unsigned HOST_WIDE_INT) 1 can be replaced with HOST_WIDE_INT_1U.
