[LLVMdev] Possible bug in getCallPreservedMask for CallingConv::Intel_OCL_BI

[Demikhovsky, Elena]

Hi,

I'm sorry for the late response, I was on vacation.

The "break" is essential here, thank you for adding it.

-  Elena


-----Original Message-----
From: Duncan P. N. Exon Smith [mailto:dexonsmith at apple.com] 
Sent: Friday, March 14, 2014 18:21
To: Philip Reames; Demikhovsky, Elena
Cc: LLVM Developers Mailing List
Subject: Re: [LLVMdev] Possible bug in getCallPreservedMask for CallingConv::Intel_OCL_BI

+elena.demikhovsky at intel.com, who added the tests in question.

On 2014 Mar 13, at 10:30, Philip Reames <listmail at philipreames.com> wrote:

> Not sure who owns this bit of code, so sending this to the general list.
> 
> It looks like there may be an unintentional fall through happening in the X86RegisterInfo::getCallPreservedMask function.  
> 
> http://llvm.org/docs/doxygen/html/X86RegisterInfo_8cpp_source.html
> 
> case CallingConv::Intel_OCL_BI
> : {
>      
> if
>  (IsWin64 && HasAVX512)
>        
> return
>  CSR_Win64_Intel_OCL_BI_AVX512_RegMask;
>      
> if
>  (Is64Bit && HasAVX512)
>        
> return
>  CSR_64_Intel_OCL_BI_AVX512_RegMask;
>      
> if
>  (IsWin64 && HasAVX)
>        
> return
>  CSR_Win64_Intel_OCL_BI_AVX_RegMask;
>      
> if
>  (Is64Bit && HasAVX)
>        
> return
>  CSR_64_Intel_OCL_BI_AVX_RegMask;
>      
> if
>  (!HasAVX && !IsWin64 && Is64Bit)
>        
> return
>  CSR_64_Intel_OCL_BI_RegMask;
>      // Missing break/return?
>  }
> 
> It _may_ be intentional - it falls through to the Cold calling convention and then general C convention, but it really looks suspicious.  
> 
> If it is intentional, please add an explicit note about the desired fall through behaviour.  I found this by inserting a new case in the switch and having tests fail.  The two tests that seem to hit the fallthrough behavor are: CodeGen/X86/avx-intel-ocl.ll and CodeGen/X86/sse-intel-ocl.ll
> 
> Philip

It looks like a bug to me.  getCalleeSavedRegs() should have essentially identical logic, and there's a break there.

I've committed r203934 and r203939 which both should have NFC.  I'll construct a testcase that exposes the problem with the missing break statement; once I have that, I can commit that change too.

Elena, does this sound right to you?
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.