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

[Vedant Kumar via Phabricator via cfe-commits]

vsk marked an inline comment as done.
vsk added a comment.

In https://reviews.llvm.org/D29369#665878, @filcab wrote:

> Why the switch to `if` instead of a fully-covered switch/case?


It lets me avoid repeating two function calls:

  switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
  case LangOptions::SOB_Defined:
    return Builder.CreateMul(Ops.LHS, Ops.RHS, "mul");
  case LangOptions::SOB_Undefined:
    if (CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow) &&
        !CanElideOverflowCheck(CGF.getContext(), Ops))
      return EmitOverflowCheckedBinOp(Ops);
    return Builder.CreateNSWMul(Ops.LHS, Ops.RHS, "mul");
  case LangOptions::SOB_Trapping:
    if (CanElideOverflowCheck(CGF.getContext(), Ops))
      return Builder.CreateNSWMul(Ops.LHS, Ops.RHS, "mul");
    return EmitOverflowCheckedBinOp(Ops);
  }



================
Comment at: test/CodeGen/ubsan-promoted-arith.cpp:56
+
+// Note: -SHRT_MIN * -SHRT_MIN can overflow.
+//
----------------
filcab wrote:
> Nit: Maybe `USHRT_MAX * USHRT_MAX` is more understandable?
Yes, I'll fix this.


================
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; }
----------------
filcab wrote:
> Maybe put the rdar ID next to the FIXME? Like this it looks like you might have written that instead of `CHECK` by mistake.
I placed the rdar ID here to communicate that the line should become a CHECK line once the bug is fixed. It will go away with D29437.


https://reviews.llvm.org/D29369