[llvm-commits] [llvm] r97064 - in /llvm/trunk: include/llvm/Target/TargetData.h lib/CodeGen/SelectionDAG/LegalizeDAG.cpp lib/CodeGen/SelectionDAG/LegalizeTypes.cpp lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp lib/Target/TargetData.cpp test/CodeGen/X86/vector-of-i1.ll

[Dan Gohman]

On Feb 25, 2010, at 2:59 AM, Duncan Sands wrote:

> Hi Dan,
> 
>> Make getTypeSizeInBits work correctly for array types; it should return
>> the number of value bits, not the number of bits of allocation for in-memory
>> storage.
>> 
>> Make getTypeStoreSize and getTypeAllocSize work consistently for arrays and
>> vectors.
>> 
>> Fix several places in CodeGen which compute offsets into in-memory vectors
>> to use TargetData information.
>> 
>> This fixes PR1784.
> 
> thanks for doing this - it is an improvement, but unfortunately it does not
> really solve PR1784.  In fact it doesn't address the most problematic issues
> at all.
> 
> For example, bitcasting between vectors and integers.  A bitcast of <8xi1>
> to i8 is currently legal.  You have decided that <8 x i1> is 8 bytes long,
> the same as [8 x i1], and is stored as 8 consecutive bytes.  This breaks the
> invariant that a bitcast from type S to type T is the same as storing an S
> to memory and loading it out again as a T.  This is a pretty fundamental
> invariant to break!  OK, suppose you decide that you don't want to break that
> invariant - you might want to say bitcast from <8xi1> to i8 is illegal, but
> a bitcast from <8xi1> to i64 is legal (same bitwidth).  But then the legality
> or not of a bitcast becomes target dependent, which is also a big no no!
> 
> As far as I can see, there are three possibilities:
> 
> (1) Drop the invariant that bitcast from S to T is the same as "store to
> memory as type S, reload from memory as type T".  This is what you chose.
> I don't think Chris will like this :)
> (2) Disallow bitcast of vectors.
> (3) Store vectors to memory in a target independent way.  This is the
> opposite approach to the one you took.
> 
> Personally I don't much like (1) ;)

I've now reverted the patch. I didn't consider that vector bitcast was an
endian-sensitive operation, which sinks the ship. I don't think disallowing
vector bitcasts is feasible. I also don't think it's worthwhile putting 
complicated bitpacking logic in the backend in the absense of anyone
legitimately expecting to use it. I don't see a reasonable way to satisfy
the new requirements within the backwards-compatible system.

Dan