leonardchan added inline comments.
================ Comment at: include/clang/Basic/TargetInfo.h:89 + // corresponding unsaturated types. + unsigned char ShortAccumFBits, ShortAccumIBits; + unsigned char AccumFBits, AccumIBits; ---------------- ebevhan wrote: > leonardchan wrote: > > ebevhan wrote: > > > leonardchan wrote: > > > > ebevhan wrote: > > > > > I suspect it's still possible to calculate the ibits based on the > > > > > fbits, even for _Accum. > > > > > > > > > > Are the unsigned values needed? The fbits for unsigned _Fract are the > > > > > same as for signed _Fract if SameFBits is set, and +1 otherwise. The > > > > > same should go for unsigned _Accum, but I don't think it's entirely > > > > > clear how this affects the integral part. > > > > Similar to the previous comment, if we choose to make SameFBits > > > > dependent on/controlled by the target, then I think it would be better > > > > for the target to explicitly specify the integral bits since there > > > > could be some cases where there may be padding (such as for unsigned > > > > _Accum types if this flag is set), and in this case, I think it should > > > > be explicit that the `bit_width != IBits + FBits`. > > > > > > > > We also can't fill in that padding for the unsigned _Accum types as an > > > > extra integral bit since the standard says that `"each signed accum > > > > type has at least as many integral bits as its corresponding unsigned > > > > accum type".` > > > > > > > > For the unsigned _Fract values, I think we can get rid of them if we > > > > choose to keep the flag instead, but it would no longer apply to > > > > unsigned _Accum types since it would allow for extra padding in these > > > > types and would conflict with the logic of `bit_width == IBits + FBits` > > > > > > > > For now, I actually think the simplest option is to keep these target > > > > properties, but have the target override them individually, with checks > > > > to make sure the values adhere to the standard. > > > Is it not the case that `bitwidth != IBits + FBits` for signed types > > > only? Signed types require a sign bit (which is neither a fractional bit > > > nor an integral bit, according to spec) but unsigned types do not, and if > > > IBits and FBits for the signed and unsigned types are the same, the MSB > > > is simply a padding bit, at least for _Accum (for _Fract everything above > > > the fbits is padding). > > > > > > My reasoning for keeping the number of configurable values down was to > > > limit the number of twiddly knobs to make the implementation simpler. > > > Granting a lot of freedom is nice, but I suspect that it will be quite > > > hard to get the implementation working properly for every valid > > > configuration. I also don't really see much of a reason for `FBits != > > > UFBits` in general. I know the spec gives a recommendation to implement > > > it that way, but I think the benefit of normalizing the signed and > > > unsigned representations outweighs the lost bit in the unsigned type. > > > > > > It's hard to say what the differences are beyond that since I'm not sure > > > how you plan on treating padding bits. In our implementation, padding > > > bits (the MSB in all of the unsigned types for us) after any operation > > > are zeroed. > > I see. The implementation would be much simpler this way. The initial idea > > was to treat the padding bits as "don't care" bits where we would mask only > > the data bits when doing operations like comparisons that look at the whole > > integer. > That does work, but for binary operations it means you might need multiple > maskings per operation. If you mask after every operation instead, you only > need one. In our implementation, the only padding bit we have is the MSB in > the unsigned types. We simply insert a 'bit clear' after every operation that > produces an unsigned type (with some exceptions). > > The E-C spec says that padding bits are 'unspecified', but > implementation-wise, they have to be defined to something in order to operate > on them. So long as you ensure that it is never possible to observe anything > but zero in those bits from a program perspective, most operations just work. > Naturally, there are ways to subvert this design. If you have a `signed > _Fract *` and cast it to an `unsigned _Fract *` and the values happen to be > negative, the padding bit will be set and you'll get exciting behavior. > > Although... I just realized that the topmost half of the `_Fract`s in your > configuration is all padding. That might make things a bit more complicated > for things like signed types... Ideally the sign bit would be extended into > the padding for those. Yeah, I initially had 2 general purpose ideas for handling the padding: - Zero/sign extend them after all operations that modify the value (+, *, <<, casting to another type, ...). This way we know the sign part of a fixed point type, regardless of how much padding there is would, always extend to the padding and sign bit of the underlying scaled integer. - Masking only the data and sign bits before all operations that read the value and don't modify it (==, !=, <, ...). This way we only compare the non-padding bits whenever performing operations that require reading all bits in the underlying integer. I wanted to choose the method that addressed operations fixed point types would least likely be used for, that is would these types be generally used more for arithmetic or comparisons. I chose the second method, but don't have any strong evidence to support this. This is also what the later patches implement but can be reworked. ================ Comment at: lib/AST/ExprConstant.cpp:9437 + } + return Success(-Value, E); + } ---------------- ebevhan wrote: > This looks very suspect to me as well... This might not respect padding on > types whose sign bit is not the MSB, such as _Fract. The same goes for the > overflow check above. > > I think it's quite important that we define the semantics of what padding > bits should contain. Probably zeroes for unsigned types and a sexted sign bit > for signed types. Initially I had the _Accum and their corresponding _Fract types the same widths to avoid casting between _Accums to a different sized _Fract (`ie short _Fract to short _Accum`), but after thinking about it, it might be simpler in the end to just have the _Fract widths have half as much as the _Accum width and have the fractional bits be one less the width (unless SameFBits is set in which case there will still be only 1 padding). This would also reduce the number of knobs that would be tweaked for different targets and help cover this case by effectively ignoring the padding bits. Repository: rC Clang https://reviews.llvm.org/D46915 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
