<br><br><div class="gmail_quote">On Sun, Feb 5, 2012 at 9:15 PM, Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com">bob.wilson@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div class="im"><div>On Feb 5, 2012, at 1:22 AM, Craig Topper wrote:</div><br><blockquote type="cite">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.<br><br>Forgot to copy the list the first time.<br></blockquote><div><br></div></div>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.</div>
</div></blockquote><div><br>The documentation for llvm_unreachable in include/llvm/Support/ErrorHandling.h has this to say<br><br>/// Marks that the current location is not supposed to be reachable.<br>/// In !NDEBUG builds, prints the message and location info to stderr.<br>
/// In NDEBUG builds, becomes an optimizer hint that the current location<br>/// is not supposed to be reachable. On compilers that don't support<br>/// such hints, prints a reduced message instead.<br>///<br>/// Use this instead of assert(0). It conveys intent more clearly and<br>
/// allows compilers to omit some unnecessary code.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div>
<div class="h5"><div><br></div><div><blockquote type="cite"><div class="gmail_quote">On Sun, Feb 5, 2012 at 12:54 AM, Duncan Sands <span dir="ltr"><<a href="mailto:baldrick@free.fr" target="_blank">baldrick@free.fr</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Craig,<br>
<div><br>
> Convert some assert(0) in default of switch statements to llvm_unreachable.<br>
<br>
</div>are they really unreachable? Or are they rather "shouldn't be reachable"?<br>
My understanding is that llvm_unreachable is for the first case, and assert<br>
for the second case.<br>
<br>
Ciao, Duncan.<br>
<div><br>
><br>
> Modified:<br>
> llvm/trunk/lib/Target/X86/X86ISelLowering.cpp<br>
><br>
> Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=149808&r1=149807&r2=149808&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=149808&r1=149807&r2=149808&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Target/X86/X86ISelLowering.cpp (original)<br>
> +++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Sat Feb 4 21:43:23 2012<br>
> @@ -4502,9 +4502,7 @@<br>
> case X86ISD::MOVSLDUP:<br>
> case X86ISD::PALIGN:<br>
> return SDValue(); // Not yet implemented.<br>
> - default:<br>
</div>> - assert(0&& "unknown target shuffle node");<br>
<div>> - return SDValue();<br>
> + default: llvm_unreachable("unknown target shuffle node");<br>
> }<br>
><br>
> Index = ShuffleMask[Index];<br>
> @@ -5813,7 +5811,7 @@<br>
> unsigned NewWidth = (NumElems == 4) ? 2 : 4;<br>
> EVT NewVT;<br>
> switch (VT.getSimpleVT().SimpleTy) {<br>
</div>> - default: assert(false&& "Unexpected!");<br>
<div>> + default: llvm_unreachable("Unexpected!");<br>
> case MVT::v4f32: NewVT = MVT::v2f64; break;<br>
> case MVT::v4i32: NewVT = MVT::v2i64; break;<br>
> case MVT::v8i16: NewVT = MVT::v4i32; break;<br>
> @@ -10577,8 +10575,7 @@<br>
> unsigned Reg = 0;<br>
> unsigned size = 0;<br>
> switch(T.getSimpleVT().SimpleTy) {<br>
> - default:<br>
</div>> - assert(false&& "Invalid value type!");<br>
<div>> + default: llvm_unreachable("Invalid value type!");<br>
> case MVT::i8: Reg = X86::AL; size = 1; break;<br>
> case MVT::i16: Reg = X86::AX; size = 2; break;<br>
> case MVT::i32: Reg = X86::EAX; size = 4; break;<br>
> @@ -10696,7 +10693,7 @@<br>
> unsigned Opc;<br>
> bool ExtraOp = false;<br>
> switch (Op.getOpcode()) {<br>
</div>> - default: assert(0&& "Invalid code");<br>
<div>> + default: llvm_unreachable("Invalid code");<br>
> case ISD::ADDC: Opc = X86ISD::ADD; break;<br>
> case ISD::ADDE: Opc = X86ISD::ADC; ExtraOp = true; break;<br>
> case ISD::SUBC: Opc = X86ISD::SUB; break;<br>
> @@ -10838,8 +10835,7 @@<br>
> DebugLoc dl = N->getDebugLoc();<br>
> switch (N->getOpcode()) {<br>
> default:<br>
</div>> - assert(false&& "Do not know how to custom type legalize this operation!");<br>
<div>> - return;<br>
> + llvm_unreachable("Do not know how to custom type legalize this operation!");<br>
> case ISD::SIGN_EXTEND_INREG:<br>
> case ISD::ADDC:<br>
> case ISD::ADDE:<br>
> @@ -12322,7 +12318,7 @@<br>
> X86TargetLowering::EmitInstrWithCustomInserter(MachineInstr *MI,<br>
> MachineBasicBlock *BB) const {<br>
> switch (MI->getOpcode()) {<br>
</div>> - default: assert(0&& "Unexpected instr type to insert");<br>
<div><div>> + default: llvm_unreachable("Unexpected instr type to insert");<br>
> case X86::TAILJMPd64:<br>
> case X86::TAILJMPr64:<br>
> case X86::TAILJMPm64:<br>
> @@ -12699,6 +12695,7 @@<br>
> case Intrinsic::x86_avx2_pmovmskb: {<br>
> // High bits of movmskp{s|d}, pmovmskb are known zero.<br>
> switch (IntId) {<br>
> + default: llvm_unreachable("Impossible intrinsic"); // Can't reach here.<br>
> case Intrinsic::x86_sse_movmsk_ps: NumLoBits = 4; break;<br>
> case Intrinsic::x86_avx_movmsk_ps_256: NumLoBits = 8; break;<br>
> case Intrinsic::x86_sse2_movmsk_pd: NumLoBits = 2; break;<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br><br clear="all"><br>-- <br>~Craig<br>
_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div></div>
</blockquote></div><br><br clear="all"><br>-- <br>~Craig<br>