[llvm] r257518 - Codegen: [PPC] Handle weighted comparisons when inserting selects.
Aaron Ballman via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 13 07:28:59 PST 2016
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.
~Aaron
>
> 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
More information about the llvm-commits
mailing list