Re: [rfa?] Implement ppc32 SYSV {extract,store} return value

[Kevin Buettner <kevinb@redhat.com>]

On Oct 4,  1:43pm, Andrew Cagney wrote:

> > Anyway, your patch looks okay to me.  Feel free to check it in.
> 
> Here is a revised version.  It's now implemented using a wrapped 
> ..._return_value.
> 
> Still ok?

Please fix the following:

+  if (TYPE_LENGTH (type) <= 8)
+    {
+      if (outval)
+	{
+	  /* This matches SVr4 PPC, it does not match gcc.  */
+	  /* The value is padded out to 8 bytes and then loaded, as
+	     two "words" into r3/r3.  */

The comment should say r3/r4, not r3/r3.  Likewise here:

+      if (inval)
+	{
+	  /* This matches SVr4 PPC, it does not match gcc.  */
+	  /* The value is padded out to 8 bytes and then loaded, as
+	     two "words" into r3/r3.  */

In a comment prior to do_ppc_sysv_return_value(), please specify
precisely which ABIs this function covers.  The function name implies
that it's only SysV, but it looks to me like it's for SysV, Altivec,
and e500.  I don't think it's worth making the function name more
verbose, but I do think it's important to list the other ABIs that
someone looking at this function needs to consider.  Otherwise, a lot
of it doesn't make sense.

I'm puzzled by the following clause:

+  if (TYPE_LENGTH (type) == 8
+      && TYPE_CODE (type) == TYPE_CODE_ARRAY
+      && TYPE_VECTOR (type)
+      && tdep->ppc_ev0_regnum >= 0)
+    {
+      if (outval)
+	{
+	  /* e500 places the return value in "ev2".  */
+	  regcache_cooked_read (regcache, tdep->ppc_ev0_regnum + 2, outval);
+	}
+      if (inval)
+	{
+	  /* e500 places the return value in "ev2".  */
+	  regcache_cooked_write (regcache, tdep->ppc_ev0_regnum + 2, inval);
+	}
+      return RETURN_VALUE_REGISTER_CONVENTION;
+    }

I realize that this was lifted from code that existed elsewhere before,
but after comparing this to the e500 ABI doc that I have, this doesn't
look right.  The ABI says:

    Functions shall return values of 64-bit DSP types (__ev64_opaque__)
    in r3.

IIRC, ev3 corresponds to the 64-bit r3, right?  So, shouldn't the
above be returning the value in ev3 (rather than ev2)?  Or am I
missing something?

This suggests that for each clause, it'd be useful to have a comment
listing which ABIs are (or are not) covered by that clause along with
additional descriptive text describing how the code corresponds to the
ABI for the non-obvious cases.  E.g, for the above, I'd get rid of the
two ``e500 places the return value in "ev2".'' comments and instead place
something like this just above the ``if (outval)'' statement:

	  /* The e500 ABI places return values for the 64-bit DSP types
	     (__ev64_opaque__) in r3.  However, in GDB-speak, ev3 corresponds
	     to the entire r3 value for e500, whereas r3 only corresponds
	     to the lower 32-bits.  So place the 64-bit DSP type's value in
	     ev3.  */

Hmm... I just realized that I don't have the Altivec ABI doc.  Do
you have a pointer?

Thanks,

Kevin

P.S. For ppc_sysv_abi_use_struct_convention(), I was considering asking
you to organize it as follows:

int
ppc_sysv_abi_use_struct_convention (int gcc_p, struct type *type)
{
   int is_struct_return;

   is_struct_return = (some concise logical expression);

   /* Verify that the ``is_struct_return'' calculation matches
      the value return implementation.  */
   assert (is_struct_return ==
           (do_ppc_sysv_return_value (type, NULL, NULL, NULL, 0) 
	     == RETURN_VALUE_STRUCT_CONVENTION));

   return is_struct_return;
}

I decided against asking you to do this for the PPC SysV ABI because,
after again looking at the ABI, I concluded that more work is required
to verify the correctness of ``some concise logical expression'' wrt the
ABI than it was to do this verification by looking at
do_ppc_sysv_return_value().  I don't think I'd care to conclude that
this will always (or even usually) be the case after only considering
a few examples.