This is the mail archive of the mailing list for the GCC 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, fortran] Implement constant-folding for TRANSFER intrinsic.

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

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