[llvm-commits] [llvm] r149808 - /llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
David Blaikie
dblaikie at gmail.com
Sun Feb 5 21:46:27 PST 2012
On Sun, Feb 5, 2012 at 9:33 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>
> On Feb 5, 2012, at 9:15 PM, Bob Wilson 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.
>
>
> Some digging shows that indeed I did miss something…. I was remembering
> Chris expressing a preference for assert:
>
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20110110/115042.html
My bet would be that in that email Chris wasn't expressing a
preference or lack thereof for assert compared to llvm_unreachable,
just for the location of the block.
"that way, in non-assert builds, the compiler still thinks everything is great."
So that the compiler doesn't complain about there being an empty block
(which is what it would see when the assert got removed & the
"default:" was immediately followed by the switch's "}" (actually
there would be a ";" in there which would probably shut the compiler
up - but a sufficiently noisy compiler might still dislike this
idiom))
Just a thought,
- David
>
> but more recently, John McCall has recommended using llvm_unreachable:
>
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110919/046498.html
I could've sworn I sent out some patches that globally changed
assert(0) to llvm_unreachable months ago, but after seeing Craig's
checkins I went looking for them & couldn't find them.
I know of at least one thread where an assert was deliberately chosen
over an unreachable (by Jim Grosbach) so as to have fallback behavior
in a release build. That sort of scares me a bit (because such a
codepath would be completely untested anyway) - & I'd really like to
be able to /only/ have llvm_unreachable-like assertions rather than
"check this in debug builds & continue silently in release builds to
see what happens" but it's a rather philosophical issue about how
people use asserts.
- David
[aside: I would sort of love to have a condition-checking
llvm_unreachable (llvm_fact? :P) (actually used for optimization like
llvm_unreachable is (compared to assert(0))) so that we could pass
assumptions through to the optimizer. Though being able to drop the
side effects of such fact-checking code might be hard to express to
LLVM]
> Sorry for the noise.
>
>
> 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
>
>
> _______________________________________________
> 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
>
More information about the llvm-commits
mailing list