filcab added a comment. Why the switch to `if` instead of a fully-covered switch/case?
In https://reviews.llvm.org/D29369#664426, @vsk wrote: > In https://reviews.llvm.org/D29369#664366, @regehr wrote: > > > Out of curiosity, how many of these superfluous checks are not subsequently > > eliminated by InstCombine? > > > I don't have numbers from a benchmark prepped. Here's what we get with the > 'ubsan-promoted-arith.cpp' test case from this patch: > > | Setup | # of overflow checks | > | unpatched, -O0 | 22 | > | unpatched, -O0 + instcombine | 7 | > | patched, -O0 | 8 | > | patched, -O0 + instcombine | 7 | > > (There's a difference between the "patched, -O0" setup and the "patched, -O0 > + instcombine" setup because llvm figures out that the symbol 'a' is 0, and > gets rid of an addition that way.) > > At least for us, this patch is still worthwhile, because our use case is `-O0 > -fsanitized=undefined`. Also, this makes less work for instcombine, but I > haven't measured the compile-time effect. Probably running mem2reg and others before instcombine would make it elide more checks. But if you're using -O0 anyway, I guess this would help anyway. ================ Comment at: test/CodeGen/ubsan-promoted-arith.cpp:56 + +// Note: -SHRT_MIN * -SHRT_MIN can overflow. +// ---------------- Nit: Maybe `USHRT_MAX * USHRT_MAX` is more understandable? ================ Comment at: test/CodeGen/ubsan-promoted-arith.cpp:99 +// CHECK-LABEL: define signext i8 @_Z4rem3 +// rdar30301609: ubsan_handle_divrem_overflow +char rem3(int i, char c) { return i % c; } ---------------- Maybe put the rdar ID next to the FIXME? Like this it looks like you might have written that instead of `CHECK` by mistake. https://reviews.llvm.org/D29369 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
