<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 30, 2016 at 4:03 PM, Mehdi Amini via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><blockquote type="cite"><span class=""><div>On Mar 30, 2016, at 3:49 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br></span><span class=""><div><br><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Wed, Mar 30, 2016 at 3:47 PM, Mehdi Amini via llvm-commits<span> </span><span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span><br>> On Mar 30, 2016, at 3:34 PM, Pete Cooper via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>><br>> Author: pete<br>> Date: Wed Mar 30 17:34:37 2016<br>> New Revision: 264945<br>><br>> URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project?rev=264945&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=264945&view=rev</a><br>> Log:<br>> Remove useless unreachable. Switch coverage already gives us this. NFC<br><br></span>Does the optimizer has a notion of the switch coverage here? Or is the front-end generating an unreachable after your change?<br></blockquote><div><br></div><div>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.<br><br>Clang avoids the possible false positive by being optimistic and assuming that the enum will only contain enumerator values.</div></div></div></span></blockquote><div><br></div><div>Clang could be more aggressive and generate an unreachable in the default-generated block of the switch instruction in this case.</div><span class=""><font color="#888888"><div><br></div></font></span></div></div></blockquote><div><br></div><div>Unfortunately that would not be correct :(</div><div><br></div><div><div>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 <a href="http://open-std.org/JTC1/SC22/WG21/docs/papers/2015/n4567.pdf">http://open-std.org/JTC1/SC22/WG21/docs/papers/2015/n4567.pdf</a>; the way it is phrased is somewhat abstruse)</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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++.</div><div><br></div><div>-- Sean Silva</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span class=""><font color="#888888"><div></div><div>-- </div><div>Mehdi</div></font></span><span class=""><div><br></div><div><br></div><br><blockquote type="cite"><div><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Also it is not totally NFC as llvm_unreachable is lowered to a real check in an assert build.<br></blockquote><div><br></div><div>I think that still qualifies under what I'd consider NFC. An assertion firing isn't functionality, as such - just bugs.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span><font color="#888888"><br>--<br>Mehdi<br></font></span><div><div><br><br><br>><br>> Modified:<br>> lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp<br>><br>> Modified: lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp<br>> URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp?rev=264945&r1=264944&r2=264945&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp?rev=264945&r1=264944&r2=264945&view=diff</a><br>> ==============================================================================<br>> --- lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp (original)<br>> +++ lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp Wed Mar 30 17:34:37 2016<br>> @@ -865,7 +865,6 @@ std::error_code Util::getSymbolTableRegi<br>> }<br>> break;<br>> }<br>> - llvm_unreachable("atom->scope() unknown enum value");<br>> }<br>><br>> std::error_code Util::addSymbols(const lld::File &atomFile,<br>><br>><br>> _______________________________________________<br>> llvm-commits mailing list<br>><span> </span><a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>><span> </span><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a></div></div></blockquote></div></div></blockquote></span></div><br></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div>