Re: [PR64164] drop copyrename, integrate into expand

[Alexandre Oliva <aoliva@redhat.com>]

On Nov 13, 2015, Jeff Law <law@redhat.com> wrote:

> On 11/11/2015 11:10 AM, Alexandre Oliva wrote:
>> On Nov 10, 2015, Jeff Law <law@redhat.com> wrote:
>> 
>>>> * function.c (assign_parm_setup_block): Right-shift
>>>> upward-padded big-endian args when bypassing the stack slot.
>>> Don't you need to check the value of BLOCK_REG_PADDING at runtime?
>>> The padding is essentially allowed to vary.
>> 
>> Well, yeah, it's the result of BLOCK_REG_PADDING that tells whether
>> upward-padding occurred and shifting is required.
>> 
>>> If you  look at the other places where BLOCK_REG_PADDING is used, it's
>>> checked in a #ifdef, then again inside a if conditional.
>> 
>> That's what I do in the patch too.
> ?  I don't see the runtime check in your patch.  I see a couple
> gcc_asserts, but no runtime check of BLOCK_REG_PADDING.

The check is not in my patch, indeed.  That's because the previous block
performs the runtime check, and it only lets through two cases: the one
we handle, and the one nobody uses.

The previous block tests this:

	  if (mode != BLKmode
#ifdef BLOCK_REG_PADDING
	      && (size == UNITS_PER_WORD
		  || (BLOCK_REG_PADDING (mode, data->passed_type, 1)
		      != (BYTES_BIG_ENDIAN ? upward : downward)))
#endif
	      )

i.e., whether we know the mode of the passed value, and its word-sized,
or its padded such that the passed value is in the lowpart of the word.
Since this is in a block that runs when size <= UNITS_PER_WORD, this
catches (and works for) all cases of default padding (when
BLOCK_REG_PADDING is not defined), and for cases in which
BLOCK_REG_PADDING is defined so as to behave like the default padding,
at least for smaller-than-word modes.

So, since this handles little-endian bytes with upward padding and
big-endian bytes with downward padding, what remains to be handled is
little-endian bytes with downward padding and big-endian bytes with
upward padding.  I found no evidence that the former is ever used
anywhere, or why anyone would ever force shifting for both REG and MEM
use, and I don't see how the code would have dealt with this case
anyway, so I left it unhandled.  The other case, big-endian bytes with
upward padding, is precisely the one that my previous patch broke on
AArch64: we have the passed values pushed to the upper part of the REG
so that it could be stored in memory as a whole word and then accessed
in the smaller mode at the same address.  After checking that this is
the case at hand, we shift the value as if we stored it in memory as a
word and loaded it in the value mode.

>> That said, the initial conditions in the if/else-if/else chain for the
>> no-larger-than-a-word case cover all of the non-BLOCK_REG_PADDING cases
>> correctly, so that, if BLOCK_REG_PADDING is not defined, we can just
>> skip the !MEM_P block altogether.  That's also the reason why we can go
>> straight to shifting when we get there.
>> 
>> I tried to document my reasoning in the comments, but maybe it was still
>> too obscure?
> Certainly seems that way.  Is it your assertion that the new code is
> what we want regardless of the *value* of REG_BLOCK_PADDING?

Sort of.  If we get to that point, there's only one reasonable value of
BLOCK_REG_PADDING (*), although there's another possible value that we
historically haven't handled and that makes very little sense to
support.  We'd have silently corrupted it before, while now we'd get an
assertion failure.  I count that as an improvement, though it's unlikely
we'd ever hit it: anyone trying to define BLOCK_REG_PADDING so as to pad
small args downward on little-endian bytes would AFAICT soon find out it
doesn't work.

(*) unless mode is BLKmode, which the newly-added code implicitly
excludes by testing that we don't have a MEM, but rather a REG.

> Essentially meaning the check in the IF is covering both cases?

Among all cases for arguments that are word-sized or smaller, the
initial IF (not present in the patch) covers all of the "usual" cases.
The remaining blocks, including the one I added, cover the remaining
handled case, namely, BIG_ENDIAN_BYTES and upward BLOCK_REG_PADDING, or
BLKmode BIG_ENDIAN_BYTES and downward BLOCK_REG_PADDING (that needs the
opposite padding when storing to big-endian mem, so that the value can
be accessed at the address in which the full word is stored).

> What am I missing here?

I agree that the way the remaining tests are written doesn't make it
clear that they're all handling a single case, which makes things
confusing.  In part, that's because they really aren't; they also deal
with BLKmode MEMs with "usual" padding.  But that's not a case that the
patch affects, because we don't have BLKmode REGs.

Any suggestions on how to improve the comments so that they convey
enough of this reasoning to make sense, without our having to write a
book :-) on the topic?

Thanks,

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer