Revert DECL_USER_ALIGN part of r241959

[Richard Sandiford]

Richard Biener <richard.guenther@gmail.com> writes:
> On Fri, Jan 5, 2018 at 9:47 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Jan 3, 2018 at 4:06 PM, Richard Sandiford
>> <richard.sandiford@linaro.org> wrote:
>>> r241959 included code to stop us increasing the alignment of a
>>> "user-aligned" variable.  This wasn't the main purpose of the patch,
>>> and I think it was just there to make the testcase work.
>>>
>>> The documentation for the aligned attribute says:
>>>
>>>   This attribute specifies a minimum alignment for the variable or
>>>   structure field, measured in bytes.
>>>
>>> The DECL_USER_ALIGN code seemed to be treating as a sort of maximum
>>> instead, but there's not really such a thing as a maximum here: the
>>> variable might still end up at the start of a section that has a higher
>>> alignment, or might end up by chance on a "very aligned" boundary at
>>> link or load time.
>>>
>>> I think people who add alignment attributes want to ensure that
>>> accesses to that variable are fast, so it seems counter-intuitive
>>> for it to make the access slower.  The vect-align-4.c test is an
>>> example of this: for targets with 128-bit vectors, we get better
>>> code without the aligned attribute than we do with it.
>>>
>>> Tested on aarch64-linux-gnu so far, will test more widely if OK.
>>
>> Works for me - I think I did this to copy behavior of code elsewhere
>> (pass_increase_alignment::increase_alignment).

OK.  I think the difference is that there we're increasing the alignment
of variables speculatively, without knowing whether it's going to help
vectorisation or not, whereas here we're more sure.

That's actually the reason I care (forgot to say).  increase_alignment
shouldn't start giving things 256-byte alignment just because we're
running the vectoriser on a target with 256-byte vectors.  We need
some kind of proof that it's going to help vectorise a particular access.

Another reason is to support vectorising search loops, where the
bound isn't known in advance.  (That's queued for GCC 9.)  If we have a
loop that operates on both a wide and a narrow type, the wide type needs
N vectors for each vector of the narrow type.  We then need to align to
N times the vector size to ensure that the accesses don't cross a page
boundary.

Part of the problem is that we don't distinguish true user alignments
from ones that have been set by GCC itself.  E.g. if increase_alignment
does set an alignment, it records that as a "user" alignment, which stops
the vectoriser from increasing it further.  But since the docs say that
the value's only a minimum, the current behaviour still feels wrong for
user attributes.

> Note I had the impression that using the aligned attribute with a "low"
> alignment is a hint to the compiler to not increase .data by overaligning
> variables.  I think there were PRs asking for that.

That's overloading the attribute so that sometimes it's a hint
to optimise accesses and sometimes it's a hint not to.  Maybe we should
support aligned+packed for variables, if people really want a fixed
alignment?  (Although that's only going to work reliably for variables
that go in their own section.)

E.g. the current behaviour triggers even for __attribute__((aligned)),
which doesn't specify a particular value.

Is the patch OK as a compromise for GCC 8?  We don't speculatively
increase the user alignment in increase_alignment, but do still increase
it if it helps to vectorise a particular loop access?

Thanks,
Richard