efriedma added inline comments.
================ Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2697 + bool SanitizeBuiltin = SanOpts.has(SanitizerKind::Builtin); + bool SanitizeOverflow = SanOpts.has(SanitizerKind::SignedIntegerOverflow); + ---------------- artem wrote: > efriedma wrote: > > Putting the behavior under both "builtin" and "signed-integer-overflow" > > feels a little weird; is there any precedent for that? > The problem is, we are instrumenting a builtin function, so on the one hand > it is reasonable to be controlled by `-fsanitize=builtin`. On the other hand, > GCC treats abs() as an arithmetic operation, so it is being instrumented by > `-fsanitize=signed-integer-overflow` (`abs(INT_MIN)` causes a negation > overflow). > > I have decided to enable instrumentation under any of the flags, but I am not > sure whether it is the right choice. I'd prefer to just follow gcc here, I think, if there isn't any strong reason to pick a different approach. ================ Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2702 + case LangOptions::SOB_Defined: + Result = Builder.CreateBinaryIntrinsic( + Intrinsic::abs, EmitScalarExpr(E->getArg(0)), Builder.getFalse(), ---------------- artem wrote: > efriedma wrote: > > Can we land the change to directly generate calls to llvm.abs() in the > > default case separately? This might end up impacting generated code for a > > variety of workloads, and I'd prefer to have a more clear bisect point. > I used llvm.abs() for code simplicity, since middle-end combines the > instructions to it anyways. I think this part can be dropped entirely because > the intrinsic is not the best possible option either (the compiler emits > conditional move and fails to elide extra sign checks if the argument is > known to be non-negative). Sure, that works too. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156821/new/ https://reviews.llvm.org/D156821 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
