This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]