This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Improve gen-libm-test.pl LIT() application
On 08/04/2016 12:45 PM, Paul E. Murphy wrote:
>
>
> On 08/04/2016 11:02 AM, Carlos O'Donell wrote:
>> On 08/04/2016 11:05 AM, Paul E. Murphy wrote:
>>> When bootstrapping float128, this exposed a number of areas where
>>> the L suffix is incorrectly applied to simple expressions when it
>>> should be applied to each constant in the expression.
>>>
>>> In order to stave off more macros in libm-test.inc, apply_lit is
>>> made slightly more intelligent. It will now split most basic
>>> expressions and apply LIT() individually to each token. As noted,
>>> it is not a perfect solution, but compromises correctness,
>>> readability, and simplicity.
>>>
>>> The above is problematic when the L real suffix is not the most
>>> expressive modifier, and the compiler complains (i.e ppc64) or
>>> silently truncates a value (i.e ppc64).
>>>
>>> * math/gen-libm-test.pl (apply_lit): Rewrite to apply
>>> LIT() to individual constants in simple expressions.
>>> (_apply_lit): Rename replaced version, and use it to
>>> apply to what appears to be a token.
>>
>> This looks like a good compromise to me.
>>
>> Question below.
>>
>>> ---
>>> math/gen-libm-test.pl | 40 +++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 39 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/math/gen-libm-test.pl b/math/gen-libm-test.pl
>>> index aa66e76..cb0a02b 100755
>>> --- a/math/gen-libm-test.pl
>>> +++ b/math/gen-libm-test.pl
>>> @@ -163,7 +163,7 @@ sub show_exceptions {
>>>
>>> # Apply the LIT(x) macro to a literal floating point constant
>>> # and strip any existing suffix.
>>> -sub apply_lit {
>>> +sub _apply_lit {
>>> my ($lit) = @_;
>>> my $exp_re = "([+-])?[[:digit:]]+";
>>> # Don't wrap something that does not look like a:
>>> @@ -180,6 +180,44 @@ sub apply_lit {
>>> return "LIT (${lit})";
>>> }
>>>
>>> +
>>> +# Apply LIT macro to individual tokens within an
>>> +# expression. This is only meant to work with
>>> +# the very simple expressions encountered like
>>> +# A (op B)*. It will not split all literals
>>> +# correctly, but suffices for this usage without
>>> +# a substantially more complex tokenizer.
>>
>> Is there any chance you can reject literals you won't correctly
>> split and raise an error?
>
> As I understand it, the only incorrect splitting occurs for
> some inputs of the form:
>
> {integer, identifier} op {integer,real}
>
> Which, will ultimately only apply LIT() to the expressions
> containing a real value as the second operand. But, LIT is
> applied to the entire expression.
>
> So you might end up passing things like "MAX_EXP+1", "0xe+1.0f"
> to _apply_lit. The former does happen, the latter is a
> constructed example.
>
> If more complicated expressions are used in libm-test.inc, or
> this workaround proven insufficient, we should refactor
> libm-test.inc to remove the need for this hack.
OK, agreed.
>>> +sub apply_lit {
>>> + my ($lit) = @_;
>>> + my @toks = ();
>>> +
>>> + # There are some constant expressions embedded in the
>>> + # test cases. To avoid writing a more complex lexer,
>>> + # we apply some fixups, and split based on simple
>>> + # operators, unfixup, then apply LIT as needed.
>>> +
>>> + # This behaves poorly on inputs like 0x0e+1.0f,
>>> + # or MAX_EXP+1, but ultimately doesn't break
>>> + # anything... for now.
>>
>> What do you mean by 'behaves poorly'? It looks like your
>> regexp's below handle it correctly.
>
> They are ultimately handled correctly, but you could still
> conceivably end up applying LIT to more than necessary.
> I.e LIT(0xe+1.0) instead of 0xe+LIT(1.0). MAX_EXP+1 is
> vacuously correct as it never needs modification.
>
> I've yet to convince myself this solution is foolproof,
> but it suffices for the current usage in libm-test.inc.
OK, please adjust the comment wording to say that instead
of "behaves poorly"
OK with that change, as long as you tested on x86_64 and
ppc64*.
--
Cheers,
Carlos.