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