Philip Reames
2014-Mar-13 17:30 UTC
[LLVMdev] Possible bug in getCallPreservedMask for CallingConv::Intel_OCL_BI
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
<http://llvm.org/docs/doxygen/html/namespacellvm_1_1CallingConv.html#a4f861731fc6dbfdccc05af5968d98974ad47327c131a0990283111588b89587cb>:
{
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140313/4dffdfa7/attachment.html>
Duncan P. N. Exon Smith
2014-Mar-14 16:20 UTC
[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 > > PhilipIt 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?
Duncan P. N. Exon Smith
2014-Mar-14 16:37 UTC
[LLVMdev] Possible bug in getCallPreservedMask for CallingConv::Intel_OCL_BI
On 2014 Mar 14, at 09:20, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:> +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?I went ahead and committed r203943 without a testcase. The accidental fallthrough is not observable.
Demikhovsky, Elena
2014-Mar-25 14:02 UTC
[LLVMdev] Possible bug in getCallPreservedMask for CallingConv::Intel_OCL_BI
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 > > PhilipIt 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.