[llvm] r257518 - Codegen: [PPC] Handle weighted comparisons when inserting selects.
Kyle Butt via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 15 11:24:16 PST 2016
Warning fix committed in r257923
On Fri, Jan 15, 2016 at 10:34 AM, Aaron Ballman <aaron at aaronballman.com>
wrote:
> On Fri, Jan 15, 2016 at 1:32 PM, Kyle Butt <iteratee at google.com> wrote:
> > Yes, an initializer would be best.
> >
> > Sorry for the delay, gmail marked this as spam despite my instructions
> not
> > to.
> >
> > I can do it if you'd prefer.
>
> Have at it. :-) Thank you!
>
> ~Aaron
>
> >
> > On Fri, Jan 15, 2016 at 8:26 AM, Aaron Ballman <aaron at aaronballman.com>
> > wrote:
> >>
> >> On Wed, Jan 13, 2016 at 11:34 AM, Benjamin Kramer <benny.kra at gmail.com>
> >> wrote:
> >> > 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 :)
> >>
> >> Good point.
> >>
> >> > 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.
> >>
> >> If no one has a better idea, I will add an initializer to SwapOps later
> >> today.
> >>
> >> ~Aaron
> >>
> >> >
> >> > - 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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160115/96076a21/attachment-0001.html>
More information about the llvm-commits
mailing list