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

[Brooks Moses]

At 08:03 AM 5/12/2007, Tobias Schlüter wrote:
>this looks good for the most part.  I have a few remarks, though.

Thanks for looking over it!

I have some replies, but no time this morning to do anything with 
them.  Paul, FX, can you have a look at this?  :)


>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.

Even for checking whether a retrieval function (such as gfc_array_size) 
returns FAILURE?  That seems convoluted, and involves assignment to an 
extra temporary variable, since it's wrong to put that function call within 
the assert....

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

I'm not sure I follow how this check can legitimately be omitted in either 
case, without producing wrong results.  The SIZE argument is optional, and 
thus can legitimately be null.  Also, before this we checked that MOLD is 
an array _or_ size != NULL; either one of those situations will cause the 
result to be an array, and so this check isn't redundant.

Or do you merely mean that the "NULL" part should be omitted, and the 
earlier version of it should be written as "!size" rather than as "size != 
NULL"?  That makes sense.

>>+       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.

I find it a bit harder to understand, personally, but I'm not very familiar 
with this sort of integer-division trick.

>>+  /* 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);

Right, yes.

>>+  buffer = gfc_getmem (buffer_size);
>
>Maybe use alloca?

Yes, that would also make sense; it's declared in gfc_simplify_transfer and 
doesn't live beyond the end of the function.

>>+static size_t
>>+size_complex (int kind)
>>+{
>>+  return kind * size_float (kind);
>             ^^^^
>Should be 2.  You also lack testcases involving COMPLEX.

Right, yes, on both counts.

>>+    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().

Hmm, right.  This is something that I don't have much idea on, and may have 
to think about a bit (or let Paul and FX think about).

>>+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?

They are currently hypothetical; even the S390 is only currently supported 
in an ASCII configuration as far as I can tell.

It's a valid concern beyond that, but I'm not sure what to do with it.

>>+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.

Yup.  This should be the reverse of the interpret function, whatever is 
done there.

>>+  /* 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.

Yes, I agree.

My inclination would be to do the cosmetic and small fixes to this and go 
ahead and commit it and then work on the alternate-character-set 
hypotheticality and the derived-type spacing stuff in follow-up patches, 
since AFAIK this doesn't cause anything to break that worked 
previously.  But that's not a strong inclination, and I'll let Paul and FX 
make the call on that.  :)

- Brooks