[lld] r264945 - Remove useless unreachable. Switch coverage already gives us this. NFC

[Sean Silva via llvm-commits]

On Wed, Mar 30, 2016 at 4:03 PM, Mehdi Amini via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
> On Mar 30, 2016, at 3:49 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, Mar 30, 2016 at 3:47 PM, Mehdi Amini via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>>
>> > On Mar 30, 2016, at 3:34 PM, Pete Cooper via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>> >
>> > Author: pete
>> > Date: Wed Mar 30 17:34:37 2016
>> > New Revision: 264945
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=264945&view=rev
>> > Log:
>> > Remove useless unreachable.  Switch coverage already gives us this.  NFC
>>
>> Does the optimizer has a notion of the switch coverage here? Or is the
>> front-end generating an unreachable after your change?
>>
>
> In GCC, the issue is that they are pessimistic about enum values - they
> assume (technically correct, of course) that an enum can have any of its
> representable values, which means that it could contain a value that isn't
> one of the enumerators & thus the switch could be bypassed entirely and so
> a return after the switch is needed.
>
> Clang avoids the possible false positive by being optimistic and assuming
> that the enum will only contain enumerator values.
>
>
> Clang could be more aggressive and generate an unreachable in the
> default-generated block of the switch instruction in this case.
>
>
Unfortunately that would not be correct :(

I ended up digging into this once because one of our game teams was getting
bad codegen for a simple example for a similar reason. The rules for C++
are sort of annoying in this situation because they basically allow any
value for the enumerator that is a bitwise OR of any of the enumerants (or
any number less than a number that can be obtained as a bitwise OR).
([dcl.enum]p8 in
http://open-std.org/JTC1/SC22/WG21/docs/papers/2015/n4567.pdf; the way it
is phrased is somewhat abstruse)

I.e. in your example, if you instead had `enum e { A, B, C };` and had
cases for A, B, and C, then clang could not assume that the default case is
unreachable: static_cast<e>(3) is also a valid value.

So yes we *could* emit unreachable in your example, but that is highly
opportunistic (limited to enums with a power-of-two number of distinct
sequential enumerators; or similar situtations where there aren't these
"phantom" enum values that the standard actually allows and *requires* us
to emit a reachable no-op default case for). I'm not sure users would want
such unpredictable performance swings on adding enumerator values.

My conclusion after my investigation was that to get "final switch"
semantics over an enum you basically have to use __builtin_unreachable() or
similar *somewhere* (either afterwards (if all cases return), or in the
default case). The compiler in general can't infer it in C++.

-- Sean Silva


> --
> Mehdi
>
>
>
>
>
>> Also it is not totally NFC as llvm_unreachable is lowered to a real check
>> in an assert build.
>>
>
> I think that still qualifies under what I'd consider NFC. An assertion
> firing isn't functionality, as such - just bugs.
>
>
>>
>> --
>> Mehdi
>>
>>
>>
>> >
>> > Modified:
>> >    lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp
>> >
>> > Modified:
>> lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp
>> > URL:
>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp?rev=264945&r1=264944&r2=264945&view=diff
>> >
>> ==============================================================================
>> > --- lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp
>> (original)
>> > +++ lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp
>> Wed Mar 30 17:34:37 2016
>> > @@ -865,7 +865,6 @@ std::error_code Util::getSymbolTableRegi
>> >     }
>> >     break;
>> >   }
>> > -  llvm_unreachable("atom->scope() unknown enum value");
>> > }
>> >
>> > std::error_code Util::addSymbols(const lld::File &atomFile,
>> >
>> >
>> > _______________________________________________
>> > 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
>>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160330/a6a69dcc/attachment.html>