On 06/14/2011 01:29 PM, Richard Guenther wrote:
> On Tue, Jun 14, 2011 at 1:16 PM, Joern Rennecke <amyl...@spamcop.net> wrote:
>> Quoting Richard Guenther <richard.guent...@gmail.com>:
>>
>>> On Tue, Jun 14, 2011 at 11:40 AM, Joern Rennecke <amyl...@spamcop.net>
>>> wrote:
>>>>
>>>> Except or the fortran/java bits (committed), this patch hasn't been
>>>> reviewed for five weeks:
>>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg00582.html
>>>
>>> A patch doing s/CUMULATIVE_ARGS*/cumulative_args_t/ only
>>> is ok.
>>
>> It's not quite that simple. The patch makes a distinction between pointers
>> to the target specific types CUMULATIVE_ARGS, and the target-independent
>> cumulative_args_t.
>>
>> Is it still OK if I selectively do the replacement where the
>> target-independent type is meant, and add a provisional
>> typedef CUMULATIVE_ARGS *cumulative_args_t to tie it together?
>>
>>> Posting compressed attached patches makes it too easy
>>> to not review things btw ...
>>
>> The mailing list size limits did't allow this patch to be posted
>> without compression.
>>
>>> After that patch the "meat" of the patch should be much much smaller
>>> and easier to review (if there is anything left besides the renaming?).
>>
>> It should be somewhat smaller, but there are lots of places where we have
>> to convert between cumulative_args_t and CUMULATIVE_ARGS *.
>> Were a target-independent interface is required, we need cumulative_args_t .
>> Where a target accesses struct components, it needs CUMULATIVE_ARGS *.
>> There are some places that just pass CUMULATIVE_ARGS * around, both in
>> rtl-centric middle-end/ rtl-optimizer code and in target code, which
>> could be electively converted. In general, I haven't done such optional
>> conversions. They could be added according to taste once the interface
>> has been straightened out. There is also a judgement call in each place
>> how closely the code is tied to the cumulative_args_t side or the
>> CUMULATIVE_ARGS * side.
>
> Hmm, I see. Maybe a GWP wants to ack your patch in whole then.
I'm not getting the point of the use of attribute((transparent_union)).
That should be removed to eliminate potential differences when compiling
other compilers, and to eliminate a potential source of bugs when
passing cumulative_args_t arguments.
Some of the formatting changes to avoid long lines are unfortunate (and
it's not done consistently); I think I'd prefer to add temporary
variables to hold the return value of pack_cumulative_args and
get_cumulative_args.
- targetm.calls.setup_incoming_varargs (&all->args_so_far,
- data->promoted_mode,
- data->passed_type,
- &varargs_pretend_bytes, no_rtl);
+ (targetm.calls.setup_incoming_varargs
+ (pack_cumulative_args (&all->args_so_far), data->promoted_mode,
+ data->passed_type, &varargs_pretend_bytes,
no_rtl));
No need for parentheses around the expression. Occurs in three places.
See previous comment about using temporary variables to avoid ugly
formatting.
I think it would be best just to minimize changes in backends as much as
possible by using the following pattern everywhere:
static void
-ix86_function_arg_advance (CUMULATIVE_ARGS *cum, enum machine_mode mode,
+ix86_function_arg_advance (cumulative_args_t cum_v, enum machine_mode mode,
const_tree type, bool named)
{
+ CUMULATIVE_ARGS *cum = get_cumulative_args (cum_v);
I.e., avoid changes such as the one in mn10300_function_arg_advance.
Also,
- if (iq2000_function_arg (&temp, mode, type, named) != 0)
+ if (iq2000_function_arg (pack_cumulative_args (&temp), mode,
type, named) != 0)
Extra tab character before !=.
- if (targetm.calls.strict_argument_naming (arg_regs_used_so_far))
+ /* ??? the code inside is a pointer increment. */
+ if (targetm.calls.strict_argument_naming (arg_regs_used_so_far_v))
What does this comment mean?
Finally, I could do without the comments squished to the right-hand side
like this:
+#include "tm.h" /* For INTMAX_TYPE, INT8_TYPE,
INT16_TYPE, INT32_TYPE,
+ INT64_TYPE, INT_LEAST8_TYPE, INT_LEAST16_TYPE,
+ INT_LEAST32_TYPE, INT_LEAST64_TYPE,
INT_FAST8_TYPE,
+ INT_FAST16_TYPE, INT_FAST32_TYPE,
INT_FAST64_TYPE,
+ BOOL_TYPE_SIZE, BITS_PER_UNIT, POINTER_SIZE,
+ INT_TYPE_SIZE, CHAR_TYPE_SIZE, SHORT_TYPE_SIZE,
+ LONG_TYPE_SIZE, LONG_LONG_TYPE_SIZE,
+ FLOAT_TYPE_SIZE, DOUBLE_TYPE_SIZE,
+ LONG_DOUBLE_TYPE_SIZE and
LIBGCC2_HAS_TF_MODE. */
(I could do without these comments entirely but I see from the archives
that Joseph requested it.)
With these changes I think it'll be OK, but I'd like to see a new patch
version first.
Bernd
