Re: [Patch,AVR] PR54222: Add fixed point support

[Georg-Johann Lay <avr at gjlay dot de>]

Weddington, Eric wrote:
> 
>> Georg-Johann Lay:
>> 
>> Hi, here is an updated patch.
>> 
>> Some functions are reworked and there is some code clean up.
>> 
>> The test results look good, there are no additional regressions.
>> 
>> The new test cases in gcc.dg/fixed-point pass except some convert-*.c for 
>> two reasons:
>> 
>> * Some test cases have a loss of precision and therefore fail. One fail is
>>   that 0x3fffffffc0000000 is compared against 0x4000000000000000 and thus
>>   fails.  Presumably its a rounding error from float.  I'd say this is not
>>   critical.
>> 
>> * PR54330: This leads to wrong code for __satfractudadq and the wrong code
>>   is already present in .expand.  From the distance this looks like a
>>   middle-end or tree-ssa problem.
>> 
>> The new patch implements TARGET_BUILD_BUILTIN_VA_LIST. Rationale is that
>> avr-fixed.md adjust some modes bit these changes are not reflected by the
>> built-in macros made by gcc. This leads to wrong code in libgcc because it
>> deduces the type layout from these built-in defines.  Thus, the respective
>>  nodes must be patches *before* built-in macros are emit.
>> 
>> The changes to LIB2FUNCS_EXCLUDE currently have no effects, this needs
>> http://gcc.gnu.org/ml/gcc-patches/2012-08/msg01580.html which is currently
>> under review.
>> 
>> Ok to install?
> 
> Hi Johann,
> 
> I have no objections to the patch, but I think it also best to wait for
> Denis to approve as well.
> 
> Based on your analysis of the test case failure you mentioned above, do you
> think we need to have some other new test cases for the AVR fixed-point
> support?

PR54330 does not look avr-related, and I don't think it makes sense to
work around that by cutting down test coverage.  That problem reminds
me of a middle-end bug where C's undefined signed overflow was applied
to IR.  This is wrong because signed overflow is only undefined if
it comes from the C source but not if it comes from IR optimizers
that mess with signed/unsigned/shifts/comparisons/carry etc. on IR.

The rounding error is no reason to cut down coverage.
The convert tests operate on powers of 2 which all can be
represented exactly by float and fixed, so there should not
be a rounding error, not even a small one.

But maybe it's not a rounding error but just a saturation artifact
because a value is saturated to 0xffffffff when converting
to/from float.  I don't know if there are other values that don't
have 2^32-1 in their representation -- I did not track all these
errors.  The test cases are insanely big and it takes time to
follow what's going on with a small target if there is no debugger.

What turned out to be unreliable is the out_fixed routine.
I fixed the problems I found and tried to add comments to make the
code more comprehensible, but there may be more problems.
I intend to rewrite that routine from scratch and replace
the original version altogether, but it might take some weeks
until I have time to return to that.

Johann