[llvm] r257518 - Codegen: [PPC] Handle weighted comparisons when inserting selects.

Aaron Ballman via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 15 10:34:41 PST 2016


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
>
>


More information about the llvm-commits mailing list