Re: [patch, fortran] Implement constant-folding for TRANSFER intrinsic.

[Tobias Schlüter <tobias dot schlueter at physik dot uni-muenchen dot de>]

Hi Brooks,

this looks good for the most part.  I have a few remarks, though.

Brooks Moses wrote:
> +  /* Calculate the size of the source.  */
> +  if (source->expr_type == EXPR_ARRAY
> +      && gfc_array_size (source, &tmp) == FAILURE)
> +    gfc_internal_error ("Failure getting length of a constant array.");

I think we're standardizing on gcc_assert even in the frontend.

> +      if (size)
             ^^^^
Above you checked size != NULL.  I'd prefer if you omitted that in both 
cases.

> +	result_length = (size_t)mpz_get_ui (size->value.integer);
> +      else
> +	{
> +	  result_length = source_size / result_elt_size;
> +	  if (result_length * result_elt_size < source_size)
> +	    result_length += 1;
> +	}

result_length = (source_size - 1) / result_elt_size + 1;

Don't know if that's harder to understand.

> +  /* Allocate the buffer to store the binary version of the source.  */
> +  buffer_size = source_size > result_size ? source_size : result_size;

buffer_size = MAX (source_size, result_size);

> +  buffer = gfc_getmem (buffer_size);

Maybe use alloca?

> +/* ---------------------------------------------------------------  */
> +/* Calculate the size of an expression.  */
> +
> +static size_t
> +size_array (gfc_expr *e)
> +{
> +  mpz_t array_size;
> +  size_t elt_size = gfc_target_expr_size (e->value.constructor->expr);
> +
> +  gfc_array_size (e, &array_size);
> +  return (size_t)mpz_get_ui (array_size) * elt_size;
> +}
> +
> +static size_t
> +size_integer (int kind)
> +{
> +  return GET_MODE_SIZE (TYPE_MODE (gfc_get_int_type (kind)));;
> +}
> +
> +
> +static size_t
> +size_float (int kind)
> +{
> +  return GET_MODE_SIZE (TYPE_MODE (gfc_get_real_type (kind)));;
> +}
> +
> +
> +static size_t
> +size_complex (int kind)
> +{
> +  return kind * size_float (kind);
            ^^^^
Should be 2.  You also lack testcases involving COMPLEX.

> +    case BT_DERIVED:
> +      ctr = e->value.constructor;
> +      for (;ctr; ctr = ctr->next)
> +	{
> +	  gcc_assert (ctr->expr != NULL);
> +	  expr_size += gfc_target_expr_size (ctr->expr);
> +	}
> +      return expr_size;

Is that so?  What about alignment, say, of INTEGER*1 components?  This 
seems to call for the middle-end's size_for_type().

> +static int
> +encode_complex (int kind, mpfr_t real, mpfr_t imaginary, unsigned char *buffer,
> +		size_t buffer_size)
> +{
> +  int size;
> +  size = encode_float (kind, real, &buffer[0], buffer_size);
> +  size += encode_float (kind, imaginary, &buffer[size], buffer_size - size);
> +  return size;
> +


> +static int
> +encode_character (int length, char *string, unsigned char *buffer,
> +		  size_t buffer_size)
> +{
> +  gcc_assert (buffer_size >= size_character (length));
> +  memcpy (buffer, string, length);
> +  return length;
> +}

What about targets with different character sets?

> +static int
> +encode_derived (gfc_expr *source, unsigned char *buffer, size_t buffer_size)
> +{
> +  gfc_constructor *ctr;
> +  int ptr = 0;
> +
> +  ctr = source->value.constructor;
> +  for (;ctr; ctr = ctr->next)
> +    {
> +      gcc_assert (ctr->expr != NULL);
> +      ptr += gfc_target_encode_expr (ctr->expr, &buffer[ptr],
> +				     buffer_size - ptr);
> +    }
> +  return ptr;
> +}

Again, alignment may interfere with this logic.

> +  /* Calculate array size from its shape and rank.  */
> +  if (result->rank == 0 || result->shape == NULL)
> +    gfc_error ("failure to obtain array size at %L", &result->where);

This should probably be caught in gfc_check_transfer, and at most 
gcc_asserted here.

Cheers,
- Tobi