On Mar 18, 2012, at 3:16 AM, Richard Sandiford wrote: > Mike Stump <mikest...@comcast.net> writes: >>> We currently only support constant integer >>> widths <= 2*HOST_BITS_PER_WIDE_INT, and the assert is correctly >>> triggering if we try to build a wider constant. >> >> Can you give me a concrete example of what will fail with the constant >> 0 generated by: >> >> return GEN_INT (i0); >> >> when the mode is 2*HOST_BITS_PER_WIDE_INT? When the mode is larger than >> this? > > I think the assert is more saying that things have already gone wrong
You apparently don't get it. Let me more forcefully state my position. No, things have not gone wrong. Let me repeat myself, again, 0 is perfectly representable in 0 bits, by induction it is perfectly representable in n+1 bits without loss. By induction, every integral size greater than 0 is also perfectly able to represent 0. If they had gone wrong, the testcase would not work, the code would not compile and I would not get valid code. > if we have reached this point. immed_double_const is only designed > to handle two HOST_WIDE_INTs, 0 fits into two HOST_WIDE_INTs. > so what exactly is the caller trying to do? Put 0 into a CONST_INT, with the result mode VOIDmode. > E.g. do we apply sign or zero extension to the 2 HOST_WIDE_INTs? Irrelevant. 0 has the same value, 0 or sign extended, honest. > If we're going to remove the assert, we need to define stuff like that. Orthogonal. The rest of the compiler defines what happens, it either is inconsistent, in which case it is by fiat, undefined, or it is consistent, in which case that consistency defines it. The compiler is free to document this in a nice way, or do, what is usually done, which is to assume everybody just knows what it does. Anyway, my point is, this routine doesn't define the data structure, and is _completely_ orthogonal to your concern. It doesn't matter if it zero extends or sign extends or is inconsistent, has bugs, doesn't have bugs, is documented, or isn't documented. In every single one of these cases, the code in the routine I am fixing, doesn't change. That is _why_ it is orthogonal. If it weren't, you'd be able to state a value for which is mattered. You can't, which is why you are wrong. If you think you are not wrong, please state a value for which it matters how it is defined. > All ones is a pretty common constant, so if I was going to guess, > I'd have expected we'd sign extend, Idle and irrelevant speculation. Let's not go down that path. > So immed_double_const (-1, -1, int_mode) would always be GEN_INT (-1). > But if we do that, we need to define the high HOST_WIDE_INT of a > CONST_DOUBLE to be sign-extended too. And at the moment we don't; Though this is orthogonal to the patch under consideration, I'd agree, defining the values as being sign extending seems reasonable to me. > the CONST_DOUBLE case of simplify_immed_subreg says: > > /* It shouldn't matter what's done here, so fill it with > zero. */ > for (; i < elem_bitsize; i += value_bit) > *vp++ = 0; And this would would have to be fixed, if _you_ wanted to define it as sign extending. Given this code, by fiat, it is defined to either be inconsistent or to be zero extending, presently. > From a quick grep, it looks like the case where I saw the immed_double_const > assert triggering -- the expansion of INTEGER_CST -- is the only case > where it could trigger. So I suppose the question shifts to the tree level. Again, irrelevant, let's not go there. I do not propose to define the data structure in my patch, nor to change the definition of the data structure. I merely propose to fix an obvious and trivial bug.
