On Sat, Jan 30, 2016 at 12:06 PM, Richard Sandiford
<rdsandif...@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 " <sell...@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 <sell...@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
