[llvm-commits] [llvm] r63495 - in /llvm/trunk: lib/Target/X86/X86CallingConv.td lib/Target/X86/X86ISelLowering.cpp lib/Target/X86/X86Subtarget.cpp test/CodeGen/X86/2009-01-25-NoSSE.ll test/CodeGen/X86/nosse-varargs.ll

[Török Edwin]

On 2009-02-02 23:43, Dan Gohman wrote:
> On Feb 2, 2009, at 12:55 PM, Török Edwin wrote:
>   
>> +    if (Is64Bit) {
>> +      // Make sure SSE2  is enabled, it is available on all X86-64  
>> CPUs.
>> +      X86SSELevel = SSE2;
>> +    }
>>     
>
> It looks like this should also check if (X86SSELevel < SSE2) so that
> it doesn't override when AutoDetectSubtargetFeatures detects an SSE
> level greater than SSE2. With that fixed, this looks good.
>   

Fixed.

>   
>>   }
>>
>> -  // If requesting codegen for X86-64, make sure that 64-bit and SSE2
>> features
>> -  // are enabled.  These are available on all x86-64 CPUs.
>> +  // If requesting codegen for X86-64, make sure that 64-bit features
>> +  // are enabled.
>>   if (Is64Bit) {
>>     HasX86_64 = true;
>>   }
>>     
>
>
> Do you think it would make sense to move this up into the else clause
> above too? I'm wondering if an assert(!Is64Bit || HasX86_64) after the
> else would be a sane sanity check.
>   

I tried that, but the assertion failed with llc -mcpu=yonah in one of
the codegen tests, so I didn't move the if for now (added the assert
though).
I've committed r63552.

Best regards,
--Edwin