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

[Duncan P. N. Exon Smith]

+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?