vectors of i1 and vectors x86 long double don't work

[llvmbot]

Bugzilla Link	1784
Version	trunk
OS	Linux
Blocks	llvm/llvm-bugzilla-archive#31265 llvm/llvm-bugzilla-archive#3037 llvm/llvm-bugzilla-archive#3352 llvm/llvm-bugzilla-archive#7303
Reporter	LLVM Bugzilla Contributor
CC	@asl,@hfinkel,@RKSimon,@sunfishcode,@darkbuck,@JonPsson,@kaomoneus,@trenouf,@Nuullll

Extended Description

I noticed that a lot of the vector code assumes that there is
no padding between vector elements. For example, the size is
assumed to be the primitive size of the element times the number
of elements. However for x86 long double the primitive size is
80 bits while elements will be spaced by 96/128 bits (depending
on the os) in order to maintain alignment. There's a similar
problem for vectors of apints.

[lattner]

Does gcc support vector of long double? Maybe it would make sense to just reject all vectors with elements that would require intra-element padding?

[llvmbot]

Yes, it looks like gcc supports vectors of long double. I compiled
this with it on x86-32 and it compiles (didn't check whether the
assembler is sensible, or how it lays out the vector).

typedef long double dvec attribute ((vector_size (24)));
void d(dvec *x);
int main() {
dvec x, y, z;
d(&x);
d(&y);
z = x + y;
d(&z);
return 0;
}

Also, we may want to support vectors of boolean as results
of vector comparisons.

[llvmbot]

To be more explicit about "There's a similar problem for vectors of apints",
consider a vector . getPrimitiveSizeInBits reckons this has size
n bits, i.e. that it has been bitpacked. Yet you can do GEP on them, and
I'm willing to bet that the GEP doesn't address individual bits! Likewise,
vast parts of codegen assume that vectors are laid out like arrays, which
in this case means that each i1 is in its own byte (also incompatible with
what getPrimitiveSizeInBits returns).

Possible solutions:
(1) bitpack vectors. This could be done but seems like a lot of work
for not much benefit. Also, I think some targets support vector compare
results producing a "vector of booleans". It would be nice if this mapped
in a convenient way to a vector of i1.
(2) lay vectors out like arrays. So elements of a vector of long double
would be spaced by 12/16 bytes depending on the target. Vectors of i1
would typically get 1 byte per i1.
(3) byte pack vectors. Here vectors of long double would have elements
spaced by 10 bytes (unaligned!). Vectors of i1 would have elements 1
byte apart.
My preference is for (2). However if GEP is disallowed for vectors,
then other schemes become more feasible.

I think Chris needs to make a policy decision here.

[llvmbot]

PS: While LegalizeDAG will happily "codegen" vectors of i1
and long double, the result is often bogus. This is why
I put asserts in LegalizeTypes in various spots where the
logic is wrong for such vectors

[llvmbot]

Sorry, should have been getBitWidth rather than getPrimitiveSizeInBits:
it is VectorType::getBitWidth that returns the number of elements * the
element size in bits. Note that codegen does the same for MVT::getSizeInBits
for a vector. The result is that vectors of i1 are considered to be
bitpacked...

[llvmbot]

*** Bug llvm/llvm-bugzilla-archive#3434 has been marked as a duplicate of this bug. ***

[llvmbot]

*** Bug llvm/llvm-bugzilla-archive#6204 has been marked as a duplicate of this bug. ***

[llvmbot]

Not to mention vectors of i24, and other types that are not naturally aligned.

[sunfishcode]

proposed patch
I propose we accept (2), lay out vectors in memory as arrays. This is good enough for most cases, and much less complicated to reason about. Note that this only affects the in-memory representation (not registers), and it only affects memory visible to the IR (not spill slots, constant pool entries, stack arguments, etc.)

People needing to bitpack vectors of i1 in IR-visible memory can bitcast the vectors to integers and store them as integers. If this becomes insufficient, we can revisit this issue.

Attached is a patch which implements the TargetData portion of this (I haven't looked at whether LegalizeTypes needs anything here yet).

[sunfishcode]

I committed the patch, plus several fixes to CodeGen to fix lowering of vectors with element padding, in r97064.

[llvmbot]

There are still several issues, see this email:

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20100222/096825.html

[llvmbot]

The decision is: vectors will be bit-packed. GEP on vectors will be disallowed.

[sunfishcode]

And for the record, r97064 was reverted, following the discussion in the email thread linked above.

[llvmbot]

I now think that vectors should be stored in memory in a target dependent manner,
which may or may not be the same as how arrays are stored in memory. This means
fixing all the places that think you can get the size of an array without target
data, not to mention users of
VectorType::getInteger
VectorType::getExtendedElementVectorType
VectorType::getTruncatedElementVectorType
VectorType::getBitWidth
since these methods are all essentially assuming that vectors are bitpacked.

An additional issue is that bitcast of vectors becomes problematic and should
probably be disallowed. The issue is that bitcast is only legal if the source
and destination types have the same bitwidth. But the bitwidth of a vector will
now depend on the target, so the legality of the IR will depend on the target,
which is not currently the case.

[sunfishcode]

I think it's a mistake to make bitcast be the operation which determines how everything else must work.

I propose the first step to solving this problem is to redefine the bitcast operator. Instead of saying it is equivalent to a store and load in the new type, I propose it be defined as:

• A bitcast operator reproduces the bits of its operand in precedence order.

• For the purposes of bitcasting, the precedence of a bit in a vector value is the
  precedence index of the bit in its element plus the element index of its
  vector element times the precision of the element type.

This is an intuitive definition, and it makes bitcast a fully target-independent operation that doesn't care about endianness. Bitcasting an integer to a vector would always put the most significant bit in the same place, which is not true under the store+load definition.

On little-endian targets, there's effectively no change. On big-endian targets, this would change the byte order of bitcasts between vectors and non-vectors and between vectors with elements of different sizes. If front-ends desire the old behavior, they can emit an explicit llvm.bswap operations.

If we accept this definition, then we can move on to consider what makes the most sense for vectors without being artificially constrained.