[llvm-commits] [llvm] r149808 - /llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
Craig Topper
craig.topper at gmail.com
Sun Feb 5 21:21:45 PST 2012
On Sun, Feb 5, 2012 at 9:15 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>
> 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.
>
The documentation for llvm_unreachable in
include/llvm/Support/ErrorHandling.h has this to say
/// Marks that the current location is not supposed to be reachable.
/// In !NDEBUG builds, prints the message and location info to stderr.
/// In NDEBUG builds, becomes an optimizer hint that the current location
/// is not supposed to be reachable. On compilers that don't support
/// such hints, prints a reduced message instead.
///
/// Use this instead of assert(0). It conveys intent more clearly and
/// allows compilers to omit some unnecessary code.
>
> 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
>
>
>
--
~Craig
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120205/025035dd/attachment.html>
More information about the llvm-commits
mailing list