[llvm-commits] [llvm] r51468 - in /llvm/trunk: include/llvm/Bitcode/LLVMBitCodes.h include/llvm/Type.h lib/AsmParser/LLLexer.cpp lib/AsmParser/llvmAsmParser.cpp.cvs lib/AsmParser/llvmAsmParser.h.cvs lib/AsmParser/llvmAsmParser.y lib/AsmParser/llvmAsmParser.y.cvs lib/Bitcode/Reader/BitcodeReader.cpp lib/Bitcode/Writer/BitcodeWriter.cpp lib/Bitcode/Writer/ValueEnumerator.cpp test/Assembler/insertextractvalue.ll test/Verifier/2006-07-11-StoreStruct.ll utils/llvm.grm

[Dan Gohman]

On May 23, 2008, at 2:45 PM, Chris Lattner wrote:

> On May 22, 2008, at 6:55 PM, Dan Gohman wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=51468&view=rev
>> Log:
>> Make structs and arrays first-class types, and add assembly
>> and bitcode support for the extractvalue and insertvalue
>> instructions and constant expressions.
>
> Woo!
>
>> +++ llvm/trunk/include/llvm/Type.h Thu May 22 20:55:30 2008
>> @@ -216,8 +216,8 @@
>>  /// is a valid type for a Value.
>>  ///
>>  inline bool isFirstClassType() const {
>> -    // Coming soon: first-class struct and array types...
>> -    return isSingleValueType();
>> +    return isSingleValueType() ||
>> +           ID == StructTyID || ID == ArrayTyID;
>>  }
>
> At this point, is it faster to just check that this *isn't* void and
> functiontype?

There's also opaque and label types. I guess that's still
4, instead of 6.

>
>
>> @@ -1966,6 +1967,48 @@
>>      GEN_ERROR("Invalid shufflevector operands");
>>    $$ = ConstantExpr::getShuffleVector($3, $5, $7);
>>    CHECK_FOR_ERROR
>> +  }
>> +  | EXTRACTVALUE '(' ConstVal IndexList ')' {
>
> Does IndexList start with a comma?  "extractvalue ( {i32} zeroinit
> 0 )" is strange, I think a comma is needed.  Also, constantexprs
> should not require types on the indices either.  It should just take a
> list of EUINT64VAL's, like getresult.

Yes, the IndexList production starts with a comma.

>> +++ llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.cpp Thu May 22
>> 20:55:30 2008
>> @@ -103,10 +103,10 @@
>>  // in the table (have low bit-width).
>>  std::stable_sort(Types.begin(), Types.end(), CompareByFrequency);
>>
>> -  // Partition the Type ID's so that the first-class types occur
>> before the
>> +  // Partition the Type ID's so that the single-value types occur
>> before the
>>  // aggregate types.  This allows the aggregate types to be dropped
>> from the
>>  // type table after parsing the global variable initializers.
>> -  std::partition(Types.begin(), Types.end(), isFirstClassType);
>> +  std::partition(Types.begin(), Types.end(), isSingleValueType);
>
> Is this required for correctness?  This could cause a significant BC
> file size increase for real world apps.  I'd try kimwitu and 176.gcc
> as examples.

It is required for correctness, given some other stuff that the
BC writer does. But it shouldn't change the BC file size;
isSingleValueType does exactly what isFirstClassType used to do;
that's why it exists even.

Dan