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 :-) Richard