[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

[Filipe Cabecinhas via Phabricator via cfe-commits]

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