<div dir="ltr">Yes, an initializer would be best.<div><br></div><div>Sorry for the delay, gmail marked this as spam despite my instructions not to.</div><div><br></div><div>I can do it if you'd prefer.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 15, 2016 at 8:26 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Jan 13, 2016 at 11:34 AM, Benjamin Kramer <<a href="mailto:benny.kra@gmail.com">benny.kra@gmail.com</a>> wrote:<br>
> On Wed, Jan 13, 2016 at 4:28 PM, Aaron Ballman via llvm-commits<br>
> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> On Tue, Jan 12, 2016 at 4:00 PM, Kyle Butt via llvm-commits<br>
>> <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>>> Author: iteratee<br>
>>> Date: Tue Jan 12 15:00:43 2016<br>
>>> New Revision: 257518<br>
>>><br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=257518&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=257518&view=rev</a><br>
>>> Log:<br>
>>> Codegen: [PPC] Handle weighted comparisons when inserting selects.<br>
>>><br>
>>> Only non-weighted predicates were handled in PPCInstrInfo::insertSelect. Handle<br>
>>> the weighted predicates as well.<br>
>>><br>
>>> This latent bug was triggered by r255398, because it added use of the<br>
>>> branch-weighted predicates.<br>
>>><br>
>>> While here, switch over an enum instead of an int to get the compiler to enforce<br>
>>> totality in the future.<br>
>>><br>
>>> Added:<br>
>>> llvm/trunk/test/CodeGen/PowerPC/2016-01-07-BranchWeightCrash.ll<br>
>>> Modified:<br>
>>> llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp<br>
>>><br>
>>> Modified: llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp<br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp?rev=257518&r1=257517&r2=257518&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp?rev=257518&r1=257517&r2=257518&view=diff</a><br>
>>> ==============================================================================<br>
>>> --- llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp (original)<br>
>>> +++ llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp Tue Jan 12 15:00:43 2016<br>
>>> @@ -744,20 +744,43 @@ void PPCInstrInfo::insertSelect(MachineB<br>
>>> "isel is for regular integer GPRs only");<br>
>>><br>
>>> unsigned OpCode = Is64Bit ? PPC::ISEL8 : PPC::ISEL;<br>
>>> - unsigned SelectPred = Cond[0].getImm();<br>
>>> + auto SelectPred = static_cast<PPC::Predicate>(Cond[0].getImm());<br>
>>><br>
>>> unsigned SubIdx;<br>
>>> bool SwapOps;<br>
>>> switch (SelectPred) {<br>
>>> - default: llvm_unreachable("invalid predicate for isel");<br>
>>> - case PPC::PRED_EQ: SubIdx = PPC::sub_eq; SwapOps = false; break;<br>
>>> - case PPC::PRED_NE: SubIdx = PPC::sub_eq; SwapOps = true; break;<br>
>>> - case PPC::PRED_LT: SubIdx = PPC::sub_lt; SwapOps = false; break;<br>
>>> - case PPC::PRED_GE: SubIdx = PPC::sub_lt; SwapOps = true; break;<br>
>>> - case PPC::PRED_GT: SubIdx = PPC::sub_gt; SwapOps = false; break;<br>
>>> - case PPC::PRED_LE: SubIdx = PPC::sub_gt; SwapOps = true; break;<br>
>>> - case PPC::PRED_UN: SubIdx = PPC::sub_un; SwapOps = false; break;<br>
>>> - case PPC::PRED_NU: SubIdx = PPC::sub_un; SwapOps = true; break;<br>
>>> + case PPC::PRED_EQ:<br>
>>> + case PPC::PRED_EQ_MINUS:<br>
>>> + case PPC::PRED_EQ_PLUS:<br>
>>> + SubIdx = PPC::sub_eq; SwapOps = false; break;<br>
>>> + case PPC::PRED_NE:<br>
>>> + case PPC::PRED_NE_MINUS:<br>
>>> + case PPC::PRED_NE_PLUS:<br>
>>> + SubIdx = PPC::sub_eq; SwapOps = true; break;<br>
>>> + case PPC::PRED_LT:<br>
>>> + case PPC::PRED_LT_MINUS:<br>
>>> + case PPC::PRED_LT_PLUS:<br>
>>> + SubIdx = PPC::sub_lt; SwapOps = false; break;<br>
>>> + case PPC::PRED_GE:<br>
>>> + case PPC::PRED_GE_MINUS:<br>
>>> + case PPC::PRED_GE_PLUS:<br>
>>> + SubIdx = PPC::sub_lt; SwapOps = true; break;<br>
>>> + case PPC::PRED_GT:<br>
>>> + case PPC::PRED_GT_MINUS:<br>
>>> + case PPC::PRED_GT_PLUS:<br>
>>> + SubIdx = PPC::sub_gt; SwapOps = false; break;<br>
>>> + case PPC::PRED_LE:<br>
>>> + case PPC::PRED_LE_MINUS:<br>
>>> + case PPC::PRED_LE_PLUS:<br>
>>> + SubIdx = PPC::sub_gt; SwapOps = true; break;<br>
>>> + case PPC::PRED_UN:<br>
>>> + case PPC::PRED_UN_MINUS:<br>
>>> + case PPC::PRED_UN_PLUS:<br>
>>> + SubIdx = PPC::sub_un; SwapOps = false; break;<br>
>>> + case PPC::PRED_NU:<br>
>>> + case PPC::PRED_NU_MINUS:<br>
>>> + case PPC::PRED_NU_PLUS:<br>
>>> + SubIdx = PPC::sub_un; SwapOps = true; break;<br>
>>> case PPC::PRED_BIT_SET: SubIdx = 0; SwapOps = false; break;<br>
>>> case PPC::PRED_BIT_UNSET: SubIdx = 0; SwapOps = true; break;<br>
>>> }<br>
>><br>
>> One of our bots now diagnoses with:<br>
>><br>
>> /opt/llvm/build-llvm/src/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp: In<br>
>> member function ‘virtual void<br>
>> llvm::PPCInstrInfo::insertSelect(llvm::MachineBasicBlock&,<br>
>> llvm::MachineBasicBlock::iterator, llvm::DebugLoc, unsigned int,<br>
>> llvm::ArrayRef<llvm::MachineOperand>, unsigned int, unsigned int)<br>
>> const’:<br>
>> /opt/llvm/build-llvm/src/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:788:45:<br>
>> warning: ‘SwapOps’ may be used uninitialized in this function<br>
>> [-Wuninitialized]<br>
>><br>
>> I think this is a false positive, but having the default:<br>
>> llvm_unreachable is likely not a terrible idea to silence the warning.<br>
>> I don't know enough about this code to feel comfortable adding it as a<br>
>> fix myself, however.<br>
><br>
> Adding the default case back would defeat the warning for missing enum<br>
> values in the switch, if that was on from the beginning it would've<br>
> prevented this bug from ever creeping in. -Wcovered-switch-default<br>
> should also complain if we add a default here :)<br>
<br>
</div></div>Good point.<br>
<span class=""><br>
> It makes silencing the warning a bit harder. Choices I see are pulling<br>
> the switch into a separate function and using early returns +<br>
> llvm_unreachable or just adding an initializer to SwapOps to pacify<br>
> compilers that are missing covered switch smarts.<br>
<br>
</span>If no one has a better idea, I will add an initializer to SwapOps later today.<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> - Ben<br>
><br>
>><br>
>>><br>
>>> Added: llvm/trunk/test/CodeGen/PowerPC/2016-01-07-BranchWeightCrash.ll<br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/PowerPC/2016-01-07-BranchWeightCrash.ll?rev=257518&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/PowerPC/2016-01-07-BranchWeightCrash.ll?rev=257518&view=auto</a><br>
>>> ==============================================================================<br>
>>> --- llvm/trunk/test/CodeGen/PowerPC/2016-01-07-BranchWeightCrash.ll (added)<br>
>>> +++ llvm/trunk/test/CodeGen/PowerPC/2016-01-07-BranchWeightCrash.ll Tue Jan 12 15:00:43 2016<br>
>>> @@ -0,0 +1,35 @@<br>
>>> +; RUN: llc <%s | FileCheck %s<br>
>>> +target datalayout = "e-m:e-i64:64-n32:64"<br>
>>> +target triple = "powerpc64le-unknown-linux-gnu"<br>
>>> +<br>
>>> +%struct.buffer_t = type { i64, i8*, [4 x i32], [4 x i32], [4 x i32], i32, i8, i8, [2 x i8] }<br>
>>> +<br>
>>> +declare i32 @__f1(i8*, %struct.buffer_t* noalias)<br>
>>> +<br>
>>> +; CHECK-LABEL: f1:<br>
>>> +define i32 @f1(i8* %__user_context, %struct.buffer_t* noalias %f1.buffer) {<br>
>>> +entry:<br>
>>> + br i1 undef, label %"assert succeeded", label %"assert failed", !prof !1<br>
>>> +<br>
>>> +"assert failed": ; preds = %entry<br>
>>> + br label %destructor_block<br>
>>> +<br>
>>> +"assert succeeded": ; preds = %entry<br>
>>> + %__f1_result = call i32 @__f1(i8* %__user_context, %struct.buffer_t* %f1.buffer) #5<br>
>>> + %0 = icmp eq i32 %__f1_result, 0<br>
>>> + br i1 %0, label %"assert succeeded11", label %"assert failed10", !prof !1<br>
>>> +<br>
>>> +destructor_block: ; preds = %"assert succeeded11", %"assert failed10", %"assert failed"<br>
>>> + %1 = phi i32 [ undef, %"assert failed" ], [ %__f1_result, %"assert failed10" ], [ 0, %"assert succeeded11" ]<br>
>>> + ret i32 %1<br>
>>> +<br>
>>> +"assert failed10": ; preds = %"assert succeeded"<br>
>>> + br label %destructor_block<br>
>>> +<br>
>>> +"assert succeeded11": ; preds = %"assert succeeded"<br>
>>> + br label %destructor_block<br>
>>> +}<br>
>>> +<br>
>>> +attributes #5 = { nounwind }<br>
>>> +<br>
>>> +!1 = !{!"branch_weights", i32 1073741824, i32 0}<br>
>>><br>
>>><br>
>>> _______________________________________________<br>
>>> llvm-commits mailing list<br>
>>> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>