richard, in the places where i delete your comments, it is because i did this and there is no further discussion. On 03/27/2013 10:54 AM, Richard Biener wrote: > this also shows the main reason I was asking for storage abstraction. > The initialization from tree is way too expensive. We have discussed this on the other thread. > +/* Convert a integer cst into a wide int expanded to BITSIZE and > + PRECISION. This call is used by tree passes like vrp that expect > + that the math is done in an infinite precision style. BITSIZE and > + PRECISION are generally determined to be twice the largest type > + seen in the function. */ > + > +wide_int > +wide_int::from_tree_as_infinite_precision (const_tree tcst, > + unsigned int bitsize, > + unsigned int precision) > +{ > > I know you have converted everything, but to make this patch reviewable > I'd like you to strip the initial wide_int down to a bare minimum. > > Only then people will have a reasonable chance to play with interface > changes (such as providing a storage abstraction). I do not really know what you mean here. While I understand that you have an idea of a pure aesthetic for an abstraction, I come from the school where the abstractions are chosen based on the needs of the clients. I see no advantage to allow you to say that this or that you do not find appealing can just go away without considering that there may be many actual places where this turns out to be exactly the correct abstraction. > +/* Check the upper HOST_WIDE_INTs of src to see if the length can be > + shortened. An upper HOST_WIDE_INT is unnecessary if it is all ones > + or zeros and the top bit of the next lower word matches. > + > + This function may change the representation of THIS, but does not > + change the value that THIS represents. It does not sign extend in > + the case that the size of the mode is less than > + HOST_BITS_PER_WIDE_INT. */ > + > +void > +wide_int::canonize () > > this shouldn't be necessary - it's an optimization - and due to value > semantics (yes - I know you have a weird mix of value semantics > and modify-in-place in wide_int) the new length should be computed > transparently when creating a new value. First, we took your advice several iterations of these patches ago. There is no more modify in place semantics!!!! Everything is completely functional. The canonize function is needed because there is a specific definition of what wide-ints look like, and this maintains that if we cannot prove that it is already canonized from the operation. > + unsigned short len; > + unsigned int bitsize; > + unsigned int precision; I removed the bitsize as a persistent field. now it has to be passed into the shift operations, the only place that needed that information. > I see we didn't get away with this mix of bitsize and precision. I'm probably > going to try revisit the past discussions - but can you point me to a single > place in the RTL conversion where they make a difference? Bits beyond > precision are either undefined or properly zero-/sign-extended. Implicit > extension beyond len val members should then provide in "valid" bits > up to bitsize (if anyone cares). That's how double-ints work on tree > INTGER_CSTs > which only care for precision, even with partial integer mode types > (ok, I never came along one of these beasts - testcase / target?). > > [abstraction possibility - have both wide_ints with actual mode and > wide_ints with arbitrary bitsize/precision] > > + enum ShiftOp { > + NONE, > + /* There are two uses for the wide-int shifting functions. The > + first use is as an emulation of the target hardware. The > + second use is as service routines for other optimizations. The > + first case needs to be identified by passing TRUNC as the value > + of ShiftOp so that shift amount is properly handled according to the > + SHIFT_COUNT_TRUNCATED flag. For the second case, the shift > + amount is always truncated by the bytesize of the mode of > + THIS. */ > + TRUNC > + }; > > I think I have expressed my opinion on this. (and SHIFT_COUNT_TRUNCATED > should vanish - certainly wide-int shouldn't care, so doesn't double-int) I understand that double int does not care, and i happen to think that that is a problem. I happen to work on three platforms where SHIFT_COUNT_TRUNCATED is not true and while I understand that this is not important for the x86, it is important that for my platforms, this be done consistently throughout the compiler. The double_int rep tries to maintain the illusion that it is doing things in infinite precision. The problem is that this is not the way the rest of the compiler works. What i am trying to do is to make the compiler behave exactly the same way, no matter which level the transformation is applied. We tell the ports that we support SHIFT_COUNT_TRUNCATED and so we need to either completely do it or not make that promise. > + enum SignOp { > + /* Many of the math functions produce different results depending > + on if they are SIGNED or UNSIGNED. In general, there are two > + different functions, whose names are prefixed with an 'S" and > + or an 'U'. However, for some math functions there is also a > + routine that does not have the prefix and takes an SignOp > + parameter of SIGNED or UNSIGNED. */ > + SIGNED, > + UNSIGNED > + }; > > See above. GCC standard practice is a 'unsigned uns' parameter. I understand that there is a large body of code that has a lot of 0s and 1s as various parameters. is there any underlying problem with trying to make the more mnemonic? I personally find, and i think that most people will agree that interfaces with a lot of required 0s and 1s very bad programming style. > + inline static wide_int from_hwi (HOST_WIDE_INT op0, const_tree type); > + inline static wide_int from_hwi (HOST_WIDE_INT op0, const_tree type, > + bool *overflow); > + inline static wide_int from_shwi (HOST_WIDE_INT op0, enum machine_mode mode); > + inline static wide_int from_shwi (HOST_WIDE_INT op0, enum machine_mode mode, > + bool *overflow); > + inline static wide_int from_uhwi (unsigned HOST_WIDE_INT op0, > + enum machine_mode mode); > + inline static wide_int from_uhwi (unsigned HOST_WIDE_INT op0, > + enum machine_mode mode, > + bool *overflow); > > way too many overloads. Besides the "raw" ones I only expect wide-ints > to be constructed from a tree INTEGER_CST or a rtx DOUBLE_INT. I see that for example in wide_int::add bitsize and precision are arbitrarily copied from the first operand. With the present way of dealing with them it would sound more logical to assert that they are the same for both operands (do both need to match?). I'd rather see wide-int being "arbitrary" precision/bitsize up to its supported storage size (as double-int is). I suppose we've been there and discussed this to death already though ;) As you have some fused operation plus sign-/zero-extend ops already the alternative is to always provide a precision for the result and treat the operands as "arbitrary" precision (that way wide_int::minus_one can simply return a sign-extended precision 1 -1). Btw, wide_int::add does not look at bitsize at all, so it clearly is redundant information. Grepping for uses of bitsize shows up only maintaining and copying around this information as well. please remove bitsize. bitsize is gone. I have added asserts so that for the symmetric binary operations like add, sub, mul there is a check that the precision of both operands is the same. for asymmetric operations like shift, the shift amount can be any size operand and for them it is perfectly proper to copy them from the first parameter. > Ok, enough for today. > > Thanks, > Richard. > >> kenny >>