On Mon, Mar 24, 2025 at 05:19:13PM -0400, James K. Lowden wrote:
> On Thu, 20 Mar 2025 13:30:27 +0100 (CET)
> Richard Biener <rguent...@suse.de> wrote:
>
> > @@ -4126,7 +4137,11 @@ count: %empty { $$ = 0; }
> > if( e ) { // verify not floating point with nonzero fraction
> > auto field = cbl_field_of(e);
> > assert(is_literal(field));
> > - if( field->data.value_of() !=
> > size_t(field->data.value_of()) ) {
> > + REAL_VALUE_TYPE vi;
> > + HOST_WIDE_INT vii = real_to_integer (TREE_REAL_CST_PTR
> > (field->data.value_of()));
> > + real_from_integer (&vi, VOIDmode, vii, SIGNED);
> > + if( !real_identical (TREE_REAL_CST_PTR
> > (field->data.value_of()),
> > + &vi) ) {
> > nmsg++;
> > error_msg(@NAME, "invalid PICTURE count '(%s)'",
> > field->data.initial );
>
> I just want to verify this still does what we want. We had
>
> > - if( field->data.value_of() != size_t(field->data.value_of()) ) {
>
> When cbl_field_t::data::value_of returned _Float128, that line compared a
> _Float128 to its value as a size_t. If the number was negative, had a
> fractional component, or was too large, the test failed.
>
> Now cbl_field_t::data::value_of returns a tree. Steps:
>
> 1. produce HOST_WIDE_INT vii from the tree
> 2. set REAL_VALUE_TYPE vi from vii
> 3. use real_identical to compare vi to the original
>
> The comment for real_identical says it's a bitwise comparison. That's not
> the same as saying the floating point value can be represented exactly as a
> size_t.
It isn't 100% the same, but the original doesn't make much sense as well,
because size_t is the host size type. E.g. if cobol1 is native x86_64-linux
compiler, size_t will be 64-bit unsigned type, if cobol1 is i686-linux ->
x86_64-linux cross-compiler, size_t will be 32-bit unsigned type.
We certainly don't want the two to behave differently.
real_to_integer truncates to integer, signed 64-bit one.
real_from_integer converts it back to REAL_VALUE_TYPE.
And real_identical indeed does bitwise comparison of all the members.
Now, if field->data.value_of() is +-INF or NAN, then the comparison would be
previously true and now is true as well. If it wasn't an exact integer,
previously true and now true as well.
If it was negative (in the HOST_WIDE_INT_MIN to -1 range), previously it
would be false, now it is true.
If it was -0.0, previously it would be true, now it is false.
If it was 0 to HOST_WIDE_INT_MAX inclusive, previously it would be false
and now is false.
If it was HOST_WIDE_INT_MAX + (size_t) 1 to ~(size_t) 0, previously it would
be false and now is false.
Too large (outside of these ranges) was true before and now as well.
So, the question is, what exactly you want to diagnose and why.
The negative case can be easily checked with real_isneg.
The -0 case can be checked with real_isnegzero.
Perhaps leaving out the non-finite and negative non--0 cases might be easier
by starting with
REAL_VALUE_TYPE *val = TREE_REAL_CST_PTR (field->data.value_of());
bool valid;
if( real_isnegzero (val) )
valid = true;
else if( !real_isfinite (val) || real_isneg (val) )
valid = false;
else
{
bool fail = false;
wide_int w = real_to_integer (val, &fail, 64);
// Or instead of 64 TYPE_PRECISION (size_type_node) or whatever you
// want to do actually.
if( fail )
valid = false;
else
{
REAL_VALUE_TYPE vi;
real_from_integer (&vi, TYPE_MODE (float128_type_mode), w, UNSIGNED);
valid = real_identical (&vi, val);
}
}
if( !valid )
{
...
error_msg(@NAME, "invalid PICTURE count '(%s)'",
...
}
And what range you want to allow as not too large in the positive direction
depends on what it wants to do. Either it should be the same thing on all
targets (e.g. 64-bit unsigned, in that case it would need to use the
wide_int real_to_integer (const REAL_VALUE_TYPE *r, bool *fail, int precision);
function instead of HOST_WIDE_INT real_to_integer (const REAL_VALUE_TYPE *r);
Or make it dependent on e.g. target size_type_node, again the same
API but perhaps passing it TYPE_PRECISION (size_type_node) instead of 64
as precision.
Jakub
