[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

[Dan Gohman]

On Feb 2, 2009, at 12:55 PM, Török Edwin wrote:
>
>> On Feb 1, 2009, at 10:15 AM, Torok Edwin wrote:
>>
>>
>>> =
>>> =
>>> ====================================================================
>>> --- llvm/trunk/lib/Target/X86/X86Subtarget.cpp (original)
>>> +++ llvm/trunk/lib/Target/X86/X86Subtarget.cpp Sun Feb  1 12:15:56
>>> 2009
>>> @@ -331,7 +331,7 @@
>>>  // are enabled.  These are available on all x86-64 CPUs.
>>>  if (Is64Bit) {
>>>    HasX86_64 = true;
>>> -#if 1
>>> +#if 0
>>>     if (X86SSELevel < SSE2)
>>>       X86SSELevel = SSE2;
>>> #endif
>>>
>>
>>
>> Can you fix the comment immediately above this to reflect
>> what the code now does?
>>
>
> See below for patch.
>
>> Also, without this code, does -march=x86-64 default to having
>> SSE2 disabled (or dependent on the host)?
>
> In my tests it defaulted to being enabled, but I think that is due to
> CPU features autodetection.
> If you supply it with a CPU it should default to SSE2, because there  
> is
> something in parsesubtargetfeatures that turns on implied features  
> of a CPU.
>
> I have a testcase that checks that by default llc -march=x86-64
> generates SSE2, and with -mattr=-sse it doesn't (sse-novarargs.ll).
>
>> Since all known
>> x86-64 hardware supports SSE2, it's convenient to have SSE2
>> enabled by default on x86-64.
>>
>
> To be safe [1], we can force the SSE level to be 2 only when
> autodetecting features. If user supplied CPU/features we don't force:
>
> [1] if you're cross-compiling from a non-SSE capable x86-32 CPU to  
> x86-64
>
> Is this patch OK to go in?
>
> Index: lib/Target/X86/X86Subtarget.cpp
> ===================================================================
> --- lib/Target/X86/X86Subtarget.cpp     (revision 63542)
> +++ lib/Target/X86/X86Subtarget.cpp     (working copy)
> @@ -322,13 +322,19 @@
>     // If feature string is not empty, parse features string.
>     std::string CPU = GetCurrentX86CPU();
>     ParseSubtargetFeatures(FS, CPU);
> +    // All X86-64 CPUs also have SSE2, however user might request no
> SSE via
> +    // -mattr, so don't force SSELevel here.
>   } else {
>     // Otherwise, use CPUID to auto-detect feature set.
>     AutoDetectSubtargetFeatures();
> +    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.

>
>   }
>
> -  // 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.

Thanks,

Dan