[llvm-commits] LLVM IR change for review or committal - resubmitted

[Dan Gohman]

On Sep 8, 2008, at 2:36 PM, Preston Gurd wrote:

> On Mon, 2008-08-09 at 14:35 -0400, Dan Gohman wrote:
>> Hi Preston,
>>
>> This looks good to me.
>
> Great!
>
>>
>> The FUNC_CODE_INST_VCMP thing is a little counter-intuitive. I guess
>> it's necessary in order to remain compatible with exiting vector
>> FUNC_CODE_INST_CMP usage though, right?
>
> Correct. Since FUNC_CODE_INST_CMP with vector arguments is used for  
> the
> vfcmp and vicmp instructions, it was necessary to add
> FUNC_CODE_INST_VCMP to support the vector forms of fcmp/icmp. It would
> probably have made more sense in the bitcode reader/writer to switch
> vicmp/vfcmp to use a newly defined bitcode operator, but I did not
> because it seemed likely that it would break existing code.
>
>>
>> Thanks!
>>
>> Dan
>
> You are most welcome! Please commit the patch when you have a chance.

I've now committed it. However, there's a problem with the bitcode
reader change for vector select, so I reverted that part of it for
now.

The problem is that the trick of passing 0 for the type to getValue
doesn't work if the value is being forward-referenced. This comes
up in the following tests, for example:
   SingleSource/Benchmarks/McGill/misr
   MultiSource/Applications/d

The bitcode reader fails saying "Invalid SELECT record".

Because a select with vector left and right values can have either
an i1 or a vector-of-i1 condition, it looks like we need to
introduce another BitCode opcode to solve this. I think the way to
do it is to have a new opcode with which we explicitly store the
type of the condition. This way, it could someday also be used
when the condition is i1, and we could phase out
FUNC_CODE_INST_SELECT and just use the new opcode for everything.

On that note, it might also make sense to rename FUNC_CODE_INST_VCMP
to FUNC_CODE_INST_CMP2 (following the example of
FUNC_CODE_INST_STORE2) and eventually migrate to a situation where
vfcmp/vicmp use the old FUNC_CODE_INST_CMP and both vector and
scalar fcmp/icmp use the new FUNC_CODE_INST_CMP2.

What do you think?

Dan