This is the mail archive of the gcc-patches@gcc.gnu.org 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] PR/14262 (ABI corner case confusion)


Ulrich Weigand wrote:
	* calls.c (load_register_parameters): If BLOCK_REG_PADDING is not
	defined, pass small BLKmode values in registers in the low-order part.

This does not look right to me. If you look at gcc-3.3, before the BLOCK_REG_PADDING support went in, you can see that the code has always called move_block_to_reg here. Changing this code to do something different may break other targets.


This also seems like an incomplete solution. What happens if you have a 12-byte structure? If this gets passed into a register-pair, then you still have the same problem, the last 4 bytes get loaded in the wrong part of the second register. Perhaps s390 does not have this problem, but other similar targets might. To solve this problem, you either need to define BLOCK_REG_PADDING, or we need a different fix.

The real problem here seems to be that we have code to handle loading BLKmode parameters, but the code is conditional on STRICT_ALIGNMENT. This is store_unaligned_arguments_into_pseudos. Since most targets that pass args in regs also define STRICT_ALIGNMENT, we never noticed that the same code was also needed in the STRICT_ALIGNMENT case. If I ifdef out the STRICT_ALIGNMENT test, I get correct code.

Looking at this some more, I think there is another subtle issue here. The BLOCK_REG_PADDING code here is reading past the end of the BLKmode object, which could be a problem if the object is at the end of mapped memory. I don't think this BLOCK_REG_PADDING code is quite right, and I am not sure why it is needed here. Ah, I see, this code was added for rs6000 (powerpc), and this is another target that does not define STRICT_ALIGNMENT. This code is safe only if the STRICT_ALIGNMENT test is removed, in which case we can only reach here for a BLKmode value which has at least word_mode alignment, in which case reading an entire word can not read past the end of mapped memory.

The attached patch needs a bit of cleanup, and the comments need to be rewritten, but otherwise I think this is the right approach. Another benefit of this is that you get better RTL in the .rtl file with my patch.

By the way, the alignment tests in store_unaligned_arguments_into_pseudos looks wrong to me too. We should not be loading BLKmode arguments into registers differently depending on their alignment. Also, if we don't handle a BLKmode arg here, then the code in load_register_paramters will not handle it correctly unless BLOCK_REG_PADDING is defined. So the alignment tests only make sense when BLOCK_REG_PADDING is defined. I don't have any proof that this is wrong, though, and we really should only worry about one problem at a time...
--
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com
Index: calls.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/calls.c,v
retrieving revision 1.323
diff -p -r1.323 calls.c
*** calls.c	26 Feb 2004 23:32:08 -0000	1.323
--- calls.c	11 Mar 2004 07:13:37 -0000
*************** expand_call (tree exp, rtx target, int i
*** 3029,3035 ****
  	 and whose alignment does not permit a direct copy into registers,
  	 make a group of pseudos that correspond to each register that we
  	 will later fill.  */
!       if (STRICT_ALIGNMENT)
  	store_unaligned_arguments_into_pseudos (args, num_actuals);
  
        /* Now store any partially-in-registers parm.
--- 3029,3035 ----
  	 and whose alignment does not permit a direct copy into registers,
  	 make a group of pseudos that correspond to each register that we
  	 will later fill.  */
!       /*      if (STRICT_ALIGNMENT) */
  	store_unaligned_arguments_into_pseudos (args, num_actuals);
  
        /* Now store any partially-in-registers parm.

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