[llvm-commits] [llvm] r149808 - /llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

[Bob Wilson]

On Feb 5, 2012, at 1:22 AM, Craig Topper wrote:

> As you can see some of them were asserting and falling through so those definitely better be unreachable or they would have weird behavior on a release build. As for the others. It's a bit difficult to quantify "shouldn't be reachable". And sticking code that attempts to handle "shouldn't be reachable" after an assert isn't really the best way to deal with that since the code can never be tested. Clang seems to have had all of its assert(0) replaced with llvm_unreachable back in September(though some have creeped in since then). So changing these isn't unprecedented.
> 
> Forgot to copy the list the first time.

Maybe I'm misremembering, but I thought the conclusion last time this came up was that llvm_unreachable should only be used in cases where it was needed to mark unreachable code, not to replace assert.  Unless I missed something, the preferred idiom for "shouldn't be reachable" switch cases is to put them at the beginning of the switch with an assert.  In a release build, they'll fall through but that's no different than any other situation where things fall apart when an assertion is not satisfied.

> On Sun, Feb 5, 2012 at 12:54 AM, Duncan Sands <baldrick at free.fr> wrote:
> Hi Craig,
> 
> > Convert some assert(0) in default of switch statements to llvm_unreachable.
> 
> are they really unreachable?  Or are they rather "shouldn't be reachable"?
> My understanding is that llvm_unreachable is for the first case, and assert
> for the second case.
> 
> Ciao, Duncan.
> 
> >
> > Modified:
> >      llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
> >
> > Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=149808&r1=149807&r2=149808&view=diff
> > ==============================================================================
> > --- llvm/trunk/lib/Target/X86/X86ISelLowering.cpp (original)
> > +++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Sat Feb  4 21:43:23 2012
> > @@ -4502,9 +4502,7 @@
> >       case X86ISD::MOVSLDUP:
> >       case X86ISD::PALIGN:
> >         return SDValue(); // Not yet implemented.
> > -    default:
> > -      assert(0&&  "unknown target shuffle node");
> > -      return SDValue();
> > +    default: llvm_unreachable("unknown target shuffle node");
> >       }
> >
> >       Index = ShuffleMask[Index];
> > @@ -5813,7 +5811,7 @@
> >     unsigned NewWidth = (NumElems == 4) ? 2 : 4;
> >     EVT NewVT;
> >     switch (VT.getSimpleVT().SimpleTy) {
> > -  default: assert(false&&  "Unexpected!");
> > +  default: llvm_unreachable("Unexpected!");
> >     case MVT::v4f32: NewVT = MVT::v2f64; break;
> >     case MVT::v4i32: NewVT = MVT::v2i64; break;
> >     case MVT::v8i16: NewVT = MVT::v4i32; break;
> > @@ -10577,8 +10575,7 @@
> >     unsigned Reg = 0;
> >     unsigned size = 0;
> >     switch(T.getSimpleVT().SimpleTy) {
> > -  default:
> > -    assert(false&&  "Invalid value type!");
> > +  default: llvm_unreachable("Invalid value type!");
> >     case MVT::i8:  Reg = X86::AL;  size = 1; break;
> >     case MVT::i16: Reg = X86::AX;  size = 2; break;
> >     case MVT::i32: Reg = X86::EAX; size = 4; break;
> > @@ -10696,7 +10693,7 @@
> >     unsigned Opc;
> >     bool ExtraOp = false;
> >     switch (Op.getOpcode()) {
> > -  default: assert(0&&  "Invalid code");
> > +  default: llvm_unreachable("Invalid code");
> >     case ISD::ADDC: Opc = X86ISD::ADD; break;
> >     case ISD::ADDE: Opc = X86ISD::ADC; ExtraOp = true; break;
> >     case ISD::SUBC: Opc = X86ISD::SUB; break;
> > @@ -10838,8 +10835,7 @@
> >     DebugLoc dl = N->getDebugLoc();
> >     switch (N->getOpcode()) {
> >     default:
> > -    assert(false&&  "Do not know how to custom type legalize this operation!");
> > -    return;
> > +    llvm_unreachable("Do not know how to custom type legalize this operation!");
> >     case ISD::SIGN_EXTEND_INREG:
> >     case ISD::ADDC:
> >     case ISD::ADDE:
> > @@ -12322,7 +12318,7 @@
> >   X86TargetLowering::EmitInstrWithCustomInserter(MachineInstr *MI,
> >                                                  MachineBasicBlock *BB) const {
> >     switch (MI->getOpcode()) {
> > -  default: assert(0&&  "Unexpected instr type to insert");
> > +  default: llvm_unreachable("Unexpected instr type to insert");
> >     case X86::TAILJMPd64:
> >     case X86::TAILJMPr64:
> >     case X86::TAILJMPm64:
> > @@ -12699,6 +12695,7 @@
> >       case Intrinsic::x86_avx2_pmovmskb: {
> >         // High bits of movmskp{s|d}, pmovmskb are known zero.
> >         switch (IntId) {
> > +        default: llvm_unreachable("Impossible intrinsic");  // Can't reach here.
> >           case Intrinsic::x86_sse_movmsk_ps:      NumLoBits = 4; break;
> >           case Intrinsic::x86_avx_movmsk_ps_256:  NumLoBits = 8; break;
> >           case Intrinsic::x86_sse2_movmsk_pd:     NumLoBits = 2; break;
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> 
> -- 
> ~Craig
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120205/60aedaea/attachment.html>