On Tue, Mar 20, 2012 at 1:26 PM, Richard Sandiford <rdsandif...@googlemail.com> wrote: > Richard Guenther <richard.guent...@gmail.com> writes: >>> I've no objection to moving the assert down to after the GEN_INT. >>> But it sounds like I'm on my own with the whole CONST_DOUBLE sign thing. >>> (That is, if we remove the assert altogether, we effectively treat the >>> number as sign-extended if it happens to fit in a CONST_INT, and >>> zero-extended otherwise. >> >> Why do we treat it zero-extended otherwise? Because we use >> gen_int_mode for CONST_INTs, which sign-extends? > > Just to make sure we're not talking past each other, I meant > moving the assert to: > > /* If this integer fits in one word, return a CONST_INT. */ > [A] if ((i1 == 0 && i0 >= 0) || (i1 == ~0 && i0 < 0)) > return GEN_INT (i0); > > <---HERE---> > > /* We use VOIDmode for integers. */ > value = rtx_alloc (CONST_DOUBLE); > PUT_MODE (value, VOIDmode); > > CONST_DOUBLE_LOW (value) = i0; > CONST_DOUBLE_HIGH (value) = i1; > > for (i = 2; i < (sizeof CONST_DOUBLE_FORMAT - 1); i++) > XWINT (value, i) = 0; > > return lookup_const_double (value); > > [A] treats i0 and i1 as a sign-extended value. So if we > removed the assert (or moved it to the suggested place): > > immed_double_const (-1, -1, 4_hwi_mode) > > would create -1 in 4_hwi_mode, represented as a CONST_INT. > The three implicit high-order HWIs are -1. That's fine, > because CONST_INT has long been defined as sign-extending > rather than zero-extending. > > But if we fail the [A] test, we go on to create a CONST_DOUBLE. > The problem is that AIUI we have never defined what happens for > CONST_DOUBLE if the mode is wider than 2 HWIs. Again AIUI, > that's why the assert is there. > > This matters because of things like the handling in simplify_immed_subreg > (which, e.g., we use to generate CONST_DOUBLE pool constants, split > constant moves in lower-subreg.c, etc.). CONST_INT is already > well-defined to be a sign-extended constant, and we handle it correctly: > > switch (GET_CODE (el)) > { > case CONST_INT: > for (i = 0; > i < HOST_BITS_PER_WIDE_INT && i < elem_bitsize; > i += value_bit) > *vp++ = INTVAL (el) >> i; > /* CONST_INTs are always logically sign-extended. */ > for (; i < elem_bitsize; i += value_bit) > *vp++ = INTVAL (el) < 0 ? -1 : 0; > break; > > But because of this assert, the equivalent meaning for > CONST_DOUBLE has never been defined, and the current code > happens to zero-extend it: > > case CONST_DOUBLE: > if (GET_MODE (el) == VOIDmode) > { > /* If this triggers, someone should have generated a > CONST_INT instead. */ > gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT); > > for (i = 0; i < HOST_BITS_PER_WIDE_INT; i += value_bit) > *vp++ = CONST_DOUBLE_LOW (el) >> i; > while (i < HOST_BITS_PER_WIDE_INT * 2 && i < elem_bitsize) > { > *vp++ > = CONST_DOUBLE_HIGH (el) >> (i - HOST_BITS_PER_WIDE_INT); > i += value_bit; > } > /* It shouldn't matter what's done here, so fill it with > zero. */ > for (; i < elem_bitsize; i += value_bit) > *vp++ = 0; > } > > So the upshot is that: > > immed_double_const (-1, -1, 4_hwi_mode) > > sign-extends i1 (the second -1), creating (-1, -1, -1, -1). But: > > immed_double_const (0, -1, 4_hwi_mode) > > effectively (as the code falls out at the moment) zero-extends it, > creating (0, -1, 0, 0). That kind of inconsistency seems wrong. > > So what I was trying to say was that if we remove the assert > altogether, and allow CONST_DOUBLEs to be wider than 2 HWIs, > we need to define what the "implicit" high-order HWIs of a > CONST_DOUBLE are, just like we already do for CONST_INT. > If we remove the assert altogether, it very much matters > what is done by that last "*vp" line. > > If Mike or anyone is up to doing that, then great. But if instead > it's just a case of handling zero correctly, moving rather than > removing the assert seems safer. > > I'm obviously not explaining this well :-)
Ok, I see what you mean. Yes, moving the assert past the GEN_INT case (though that is specifically meant to deal with the VOIDmode case I think?) is ok. Thanks, Richard. > Richard