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

[Paul Thomas]

Tobi and Brooks,
>
> Thanks for looking over it!
Indeed, I second that.  As usual, you have given it a thorough going over.
>
> I have some replies, but no time this morning to do anything with 
> them.  Paul, FX, can you have a look at this?  :)
Indeed you might smile.  I have some nasty refereeing jobs to attend to 
this weekend.  However...
> ...snip...
>>
>> 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....
I'm inclined to not particularly care either way.  At risk of causing it 
to happen, I would bet that we never see this error.
>
>>> +      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.
We have a thorough mix of both styles:)  From my perspective both are 
equally clear and they produce the same code, do they not?
>
>>> +       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.
I think that we can take that one!
>
>>> +  /* 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.
Agreed.
>
>>> +  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.
OK.
>
>
>>> +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.
Indeed.  Also, we need testcases for REAL10/REAL16.  I have the patch 
back on my tree, so this is do-able, now.
>
>>> +    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).
I'll check the associated variables.  The bits of gcc in which Brooks 
was wading have all the required information.  I'll set about finding 
out.  If it's not quick and easy, I would vote for erecting TODOs.
>
>>> +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.
Ignore it until the first PR comes along.
>
>>> +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.
Indeed.
>
>>> +  /* 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.
OK
>
> 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.  :)
I agree.
>
>
Paul
>
>