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?
> 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
