[Patch, MIPS] Fix PR target/68273, passing args in wrong regs

[Richard Biener]

On Sat, Jan 30, 2016 at 12:06 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> I'm not sure this patch is safe.  The received wisdom used to be that
> ABIs should be defined in terms of types, not modes, since types
> represent the source code while modes are an internal GCC concept
> that could change over time (although in practice the barrier for
> that is pretty high).
>
> The patch that introduced the line being patched was part of a series
> that fixed ABI incompatibilities with MIPSpro, and IIRC some of the
> incompatibilties really were due to us looking at modes when we should
> have been looking at types.
>
> So...
>
> "Steve Ellcey " <sellcey@imgtec.com> writes:
>> This is a patch for PR 68273, where MIPS is passing arguments in the wrong
>> registers.  The problem is that when passing an argument by value,
>> mips_function_arg_boundary was looking at the type of the argument (and
>> not just the mode), and if that argument was a variable with extra alignment
>> info (say an integer variable with 8 byte alignment), MIPS was aligning the
>> argument on an 8 byte boundary instead of a 4 byte boundary the way it should.
>>
>> Since we are passing values (not variables), the alignment of the variable
>> that the value is copied from should not affect the alignment of the value
>> being passed.
>
> ...to me this suggests we might have a middle-end bug.  The argument
> seems to be that the alignment of the type being passed in cannot
> reasonably be used to determine the ABI for semantic reasons
> (rvalues don't have meaningful alignment).  Doesn't that mean that
> we're passing the wrong type to the hook?  The point of the hook
> is to say how an ABI handles a particular type, so if we have a type
> variant that the ABI can't reasonably handle directly for language
> reasons, shouldn't we be passing the main variant instead?

Actually I'd say it's a frontend bug then setting DECL_ARG_TYPE to the
wrong type.  Quoting:

/* For a PARM_DECL, records the data type used to pass the argument,
   which may be different from the type seen in the program.  */
#define DECL_ARG_TYPE(NODE) (PARM_DECL_CHECK (NODE)->decl_common.initial)

if the type doesn't end up coming from DECL_ARG_TYPE but the value
then I'd agree - we should always look at the main variant here.  Expansion is
quite twisty here so catching all cases is going to be interesting as well as
evaluating ABI side-effects - that's much easier to determine when you change
one target at a time...

Richard.

>> This patch fixes the problem and it could change what registers arguments
>> are passed in, which means it could affect backwards compatibility with
>> older programs.  But since the current behaviour is not compliant with the
>> MIPS ABI and does not match what LLVM does, I think we have to make this
>> change.  For the most part this will only affect arguments which are copied
>> from variables that have non-standard alignments set by the aligned attribute,
>> however the SRA optimization pass can create aligned variables as it splits
>> aggregates apart and that was what triggered this bug report.
>>
>> This is basically the same bug as the ARM bug PR 65956 and the fix is
>> pretty much the same too.
>>
>> Rather than create MIPS specific tests that check the use of specific
>> registers I created two tests to put in gcc.c-torture/execute that
>> were failing before because GCC on MIPS was not consistent in where
>> arguments were passed and which now work with this patch.
>>
>> Tested with mips-mti-linux-gnu and no regressions.  OK to checkin?
>
> Given the argument about LLVM, I think it would also be worth running
> the compat testsuite with clang as the alt compiler both before and
> after the patch to see whether we regress.
>
>> 2016-01-29  Steve Ellcey  <sellcey@imgtec.com>
>>
>>       PR target/68273
>>       * config/mips/mips.c (mips_function_arg_boundary): Fix argument
>>       alignment.
>>
>>
>>
>> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
>> index dd54d6a..ecce3cd 100644
>> --- a/gcc/config/mips/mips.c
>> +++ b/gcc/config/mips/mips.c
>> @@ -5643,8 +5643,9 @@ static unsigned int
>>  mips_function_arg_boundary (machine_mode mode, const_tree type)
>>  {
>>    unsigned int alignment;
>> -
>> -  alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
>> +  alignment = type && mode == BLKmode
>> +           ? TYPE_ALIGN (TYPE_MAIN_VARIANT (type))
>> +           : GET_MODE_ALIGNMENT (mode);
>>    if (alignment < PARM_BOUNDARY)
>>      alignment = PARM_BOUNDARY;
>>    if (alignment > STACK_BOUNDARY)
>
> We need to be careful of examples like:
>
>   struct __attribute__ ((aligned (8))) s { _Complex float x; };
>   void foo (struct s *ptr, struct s val) { *ptr = val; }
>
> "x" gets SCmode, which has an alignment of 4.  And it's OK for TYPE_MODE
> to have a smaller alignment than the type -- it's just not allowed to
> have a larger alignment (and even that restriction only applies because
> this is a STRICT_ALIGNMENT target).  So the structure itself inherits
> this SCmode.
>
> The patch therefore changes how we handle foo() for -mabi=32 -msoft-float.
> Before the patch "val" is passed in $6 and $7.  After the patch it's
> passed in $5 and $6.  clang behaves like the unpatched GCC.
>
> If instead we use:
>
>   struct __attribute__ ((aligned (8))) s { float x; float y; };
>   void foo (struct s *ptr, struct s val) { *ptr = val; }
>
> then the structure has BLKmode and the alignment is honoured both before
> and after the patch.
>
> There's no real ABI reason for handling the two cases differently.
> The fact that one gets BLKmode and the other doesn't is down
> to GCC internals.
>
> We also have to be careful about the STRICT_ALIGNMENT thing.
> At the moment that's hard-coded to 1 for MIPS, but it's possible that
> it could become configurable in future, like it is for aarch64 and
> rs6000.  !STRICT_ALIGNMENT allows TYPE_MODE to have a larger
> alignment than the type, so underaligned structures would get modes
> when they didn't previously.  That would in turn change how they're
> passed as arguments.
>
> Thanks,
> Richard