[PATCH] D135011: Add builtin_elementwise_sin and builtin_elementwise_cos

scanon added inline comments. Comment at: clang/docs/LanguageExtensions.rst:607 + T __builtin_elementwise_cos(T x)return the ratio of the adjacent side length over thefloating point types + hypoteneuse side

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

scanon added a comment. Wearing my compiler user hat, I would much rather use additive -mfeature than have to specify these as -march+feature, even when using a build system that nominally handles this stuff, because I frequently want to be able to compile one specific file with "whatever the

[PATCH] D129231: [Builtins] Do not claim all libfuncs are readnone with trapping math.

scanon added a comment. In any event, from the standpoint of the C(23) language, these operations do not set inexact, so I believe that it is appropriate to optimize them as if they do not set inexact. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D129231: [Builtins] Do not claim all libfuncs are readnone with trapping math.

scanon added a comment. >> Looking at implementations of these functions, it looks like GNU libm >> doesn't raise inexact, but the bionic libm does. I think I'm leaning towards >> marking all of them as "fng" as it's the more cautious of the two. > > Hmm, bionic's behavior sounds a bit

[PATCH] D129231: [Builtins] Do not claim all libfuncs are readnone with trapping math.

scanon added a comment. The CFP working group and C23 have since clarified this in Annex F: > The returned value is exact and is independent of the current rounding > direction mode. They never set inexact on an implementation that claims 60559 conformance. The only flag that these operations

[PATCH] D129231: [Builtins] Do not claim all libfuncs are readnone with trapping math.

scanon added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:1409 -LIBBUILTIN(round, "dd", "fnc", "math.h", ALL_LANGUAGES) -LIBBUILTIN(roundf, "ff", "fnc", "math.h", ALL_LANGUAGES) -LIBBUILTIN(roundl, "LdLd", "fnc", "math.h", ALL_LANGUAGES)

[PATCH] D120395: [X86] Prohibit arithmetic operations on type `__bfloat16`

scanon added a comment. There's a lot of churn around proposed "solutions" on this and related PR, but not a very clear analysis of what the problem we're trying to solve is. Concretely, what are the semantics that we want for the BF16 types and intrinsics? Unlike the other floating-point

[PATCH] D113107: Support of expression granularity for _Float16.

scanon added a comment. In D113107#3138671 , @rjmccall wrote: >> Basically agree with everything John said, with a note that #3 is not quite >> FP_CONTRACT, which allows evaluating an expression as if intermediate steps >> were infinitely-precise, but

[PATCH] D113107: Support of expression granularity for _Float16.

scanon added a comment. In D113107#3138415 , @rjmccall wrote: > I think we keep dancing around this in this review, so let me go back and > start from the basics. There are four approaches I know of for evaluating a > homogeneous `_Float16` expression

[PATCH] D111986: [Clang] Add elementwise abs builtin.

scanon accepted this revision. scanon added a comment. This revision is now accepted and ready to land. Two minor questions, but also LGTM as is. Comment at: clang/lib/Sema/SemaChecking.cpp:16667 + + if (!TyA->getAs() && !ConstantMatrixType::isValidElementType(TyA)) +

[PATCH] D111986: [Clang] Add elementwise abs builtin.

scanon added a comment. What's the rationale for making abs undefined on the minimum value? AFAIK every actual simd implementation defines the result and they agree (and even if one didn't, it would be pretty easy to get the "right" result. Introducing UB here just seems like punishing users

[PATCH] D111529: Specify Clang vector builtins.

scanon accepted this revision. scanon added a comment. This revision is now accepted and ready to land. I'm happy with this now. Comment at: clang/docs/LanguageExtensions.rst:552 +operation(x, y) as pairwise tree reduction to the input. The pairs are formed +by concatenating

[PATCH] D111529: Specify Clang vector builtins.

scanon added inline comments. Comment at: clang/docs/LanguageExtensions.rst:565 + NaNs, fmax() return a NaN. + ET __builtin_reduce_add(VT a) \+ integer and floating

[PATCH] D111529: Specify Clang vector builtins.

scanon added inline comments. Comment at: clang/docs/LanguageExtensions.rst:565 + NaNs, fmax() return a NaN. + ET __builtin_reduce_add(VT a) \+ integer and floating

[PATCH] D111529: Specify Clang vector builtins.

scanon added inline comments. Comment at: clang/docs/LanguageExtensions.rst:538 + T __builtin_elementwise_rint(T x) return the integral value nearest to x (according to the floating point types +prevailing rounding mode) in

[PATCH] D102494: [Clang, Driver] Default to Darwin_libsystem_m veclib on iOS based targets.

scanon added inline comments. Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2492 + // Darwin_libsystem_m for iOS based targets. + if (isTargetIOSBased() && !DriverArgs.hasArgNoClaim(options::OPT_fveclib)) +CC1Args.push_back("-fveclib=Darwin_libsystem_m");

[PATCH] D97854: [RFC][nsan] A Floating-point numerical sanitizer.

scanon added a comment. Is there a mechanism to instruct the sanitizer to ignore a specific expression or function? From a cursory reading, I am mildly concerned about a deluge of false positives from primitives that compute exact (or approximate) residuals; these are acting to eliminate or

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

scanon accepted this revision. scanon added a comment. This revision is now accepted and ready to land. I'm fine with this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 ___ cfe-commits mailing list

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

scanon added a comment. Strictly speaking, fp-contract=fast probably should have been a separate flag entirely (since there's no _expression_ being contracted in fast). Unfortunately, that ship has sailed, and it does constrain our ability to choose an accurate name somewhat. What if we just

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

scanon added a comment. I do not much like faststd, as there's nothing "standard" about it. I do not, however, have a better suggestion off the top of my head. Let's pause and consider the name a little bit longer, please? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

scanon added a comment. (If you tell GCC to respect the pragma via -std=c17 or similar, then -ffp-contract=fast overrides it just like clang's current behavior: https://godbolt.org/z/5dxxGb) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

scanon added a comment. GCC doesn't respect the pragma, so "what other compilers do" is not a particularly useful metric. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 ___ cfe-commits mailing list

[PATCH] D89699: [ExtVector] Make .even/.odd/.lo/.hi return vectors for single elements.

scanon added a comment. I guess the counterargument here would be that `.x` does not produce an extvector(1), and there is at least a plausible argument that `.x` should be the same as `.lo` for a two-element vector. I'm not really convinced by this, but it's not totally outrageous.

[PATCH] D89699: [ExtVector] Make .even/.odd/.lo/.hi return vectors for single elements.

scanon added a comment. I'm fairly certain that this will cause some breaks internally at Apple, but I'm also pretty sure that it's a step in the right direction, and we should just sign up to fix any issues it causes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D85031: [builtins] Unify the softfloat division implementation

scanon added inline comments. Comment at: compiler-rt/lib/builtins/fp_div_impl.inc:99 + // 0 < x0(b) < 1 + // abs(x0(b) - 1/b) <= 3/4 - 1/sqrt(2) + sepavloff wrote: > atrosinenko wrote: > > sepavloff wrote: > > > This estimation is absent from the original

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

scanon added a comment. > Prior to this change contract was never generated in the case of in-statement > contraction only, instead clang was emitting llvm.fmuladd to inform the > backend that only those were eligible for contraction. From a correctness > perspective I think this was perfectly

[PATCH] D78827: Add support for #pragma clang fp reassociate(on|off) -- floating point control of associative math transformations

scanon added a comment. TS18661-5 is quite vague on what the intended semantics for the pragma are. These pragmas are intended to be bindings of clause 10.4 of IEEE 754, which is also pretty wishy-washy on the whole, but it's worth noting that clause 10 is titled *expression evaluation*

[PATCH] D78827: Add support for #pragma clang fp reassociate(on|off) -- floating point control of associative math transformations

scanon accepted this revision. scanon added a comment. This revision is now accepted and ready to land. My concerns have been addressed. Thanks for bearing with me, Melanie! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78827/new/

[PATCH] D78827: Add support for #pragma clang fp reassociate(on|off) -- floating point control of associative math transformations

scanon added a comment. (Please get one additional sign off before committing; I'm mainly signing off on the numerics model aspect). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78827/new/ https://reviews.llvm.org/D78827

[PATCH] D78827: Add support for #pragma clang fp reassociate(fast|off) -- floating point control of associative math transformations

scanon added a comment. > I don't think the C standard is likely to ever bless reassociative FP math > with an expression-local restriction. Steve, do you actually think that would > be a useful optimization mode? I think it's pretty unlikely that C would do this, as well. It is plausibly a

[PATCH] D78827: Add support for #pragma clang fp reassociate(on|off) -- floating point control of associative math transformations

scanon added inline comments. Comment at: clang/docs/LanguageExtensions.rst:3182 +enabled for the translation unit with the ``-fassociative-math`` flag. +The pragma can take two values: ``on`` and ``off``. + mibintc wrote: > scanon wrote: > > rjmccall wrote: > >

[PATCH] D78827: Add support for #pragma clang fp reassociate(on|off) -- floating point control of associative math transformations

scanon added inline comments. Comment at: clang/docs/LanguageExtensions.rst:3182 +enabled for the translation unit with the ``-fassociative-math`` flag. +The pragma can take two values: ``on`` and ``off``. + rjmccall wrote: > Do you want to add an example here?

[PATCH] D78190: Add Bfloat IR type

scanon added inline comments. Comment at: llvm/docs/LangRef.rst:2896 + * - ``bfloat`` + - 16-bit brain floating-point value (8-bit mantissa) + rjmccall wrote: > rjmccall wrote: > > scanon wrote: > > > bfloat and fp128 should agree w.r.t. whether or not the

[PATCH] D78190: Add Bfloat IR type

scanon requested changes to this revision. scanon added inline comments. This revision now requires changes to proceed. Comment at: llvm/docs/LangRef.rst:3240 +double are represented using the 16-digit form shown above (which matches the +IEEE754 representation for double); half

[PATCH] D74183: [IRGen] Add an alignment attribute to underaligned sret parameters

scanon added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2077 +getDataLayout().getABITypeAlignment(getTypes().ConvertType(RetTy))) + SRETAttrs.addAlignmentAttr(Align); ArgAttrs[IRFunctionArgs.getSRetArgNo()] = rjmccall wrote: >

[PATCH] D69878: Consoldiate internal denormal flushing controls

scanon added inline comments. Comment at: llvm/docs/LangRef.rst:1834 +``"denormal-fp-math-f32"`` + Same as ``"denorm-fp-math-f32"``, except for float types. If both + are present, this overrides ``"denorm-fp-math"``. Can you clarify this a little bit? I'd

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

scanon accepted this revision. scanon added a comment. This revision is now accepted and ready to land. I believe that the code can still be simplified somewhat, but that it's correct as-is for `float`, `double`, and `long double`. I'll take an AI to follow-up on future improvements, and let's

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

scanon added a comment. In D66836#1649846 , @zoecarver wrote: > > Dead link. > > Here: https://godbolt.org/z/AjBHYq Yes, conversion of `numeric_limits::max` to `double` rounds to a value out of range for `long long`. That's not what I'm talking about.

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

scanon added a comment. > Eric showed me this link https://godbolt.org/z/AjBHYqv Dead link. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66836/new/ https://reviews.llvm.org/D66836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

scanon requested changes to this revision. scanon added inline comments. This revision now requires changes to proceed. Comment at: include/math.h:1586 +return _Lim::min(); + } + return static_cast<_IntT>(__trunc_r); EricWF wrote: > scanon wrote: > > If

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

scanon added inline comments. Comment at: include/math.h:1582 + const _RealT __trunc_r = __builtin_trunc(__r); + if (__trunc_r >= ::nextafter(static_cast<_RealT>(_MaxVal), INFINITY)) { +return _Lim::max(); EricWF wrote: > scanon wrote: > > zoecarver wrote:

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

scanon added inline comments. Comment at: test/libcxx/numerics/truncating_cast.pass.cpp:36 + {static_cast(Lim::max()) + 1, Lim::max(), false}, + {static_cast(Lim::max()) + 1024, Lim::max(), false}, + }; Probably should test

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

scanon added a comment. I would tend to write this function in the following form: // set up lower bound and upper bound if (r > upperBound) r = upperBound; if (!(r >= lowerBound)) r = lowerBound; // NaN is mapped to l.b. return static_cast(r); I prefer to avoid the explicit trunc call,

[PATCH] D65753: Builtins: Add some v2f16 variants

scanon added a comment. (Ideally we would just call them e.g. __builtin_floor, but that would be source-breaking. __builtin_tgmath_floor seems like a good compromise.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65753/new/ https://reviews.llvm.org/D65753

[PATCH] D65753: Builtins: Add some v2f16 variants

scanon added a comment. Strongly agree with what @rjmccall said. If we can make these generic builtins instead of ending up with O(100) variants of each math operation, that would make life immensely nicer. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65753/new/

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

scanon accepted this revision. scanon added a comment. This revision is now accepted and ready to land. LGTM. Please get at least one additional reviewer's approval before merging, as this is a corner of clang that I don't work on often. CHANGES SINCE LAST ACTION

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

scanon requested changes to this revision. scanon added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Sema/SemaChecking.cpp:11429 + S.Context.getFloatTypeSemantics(QualType(TargetBT, 0))); +

[PATCH] D57188: Disable _Float16 for non ARM/SPIR Targets

scanon added a subscriber: ab. scanon added a comment. > do we want to support _Float16 anywhere else? ARM is the only in-tree target with a defined ABI that I'm aware of. > Do we need to lock down an ABI here for i386/x86_64 in advance of those gears > turning in the outer world? We

[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.

scanon added inline comments. Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:187 + assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON); + assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON); + assert(std::fabs(v[2] - 2.0f) < FLT_EPSILON);

[PATCH] D52835: [Diagnostics] Check integer to floating point number implicit conversions

scanon added inline comments. Comment at: lib/Sema/SemaChecking.cpp:110 +/// are usually useless +static unsigned AdjustPrecision(unsigned precision) { + return (precision * 59 + 195) / 196; erichkeane wrote: > Hmm I don't terribly understand how this

[PATCH] D46926: [Fixed Point Arithmetic] Conversion between Fixed Point and Floating Point Numbers

scanon requested changes to this revision. scanon added a comment. This revision now requires changes to proceed. IIRC the optimization of divide-by-power-of-two --> multiply-by-inverse does not occur at -O0, so it would be better to multiply by 2^(-fbits) instead. Repository: rC Clang

[PATCH] D46042: Cap vector alignment at 16 for all Darwin platforms

scanon added a comment. In https://reviews.llvm.org/D46042#1088044, @ab wrote: > So, this makes sense to me, but on x86, should we also be worried about the > fact that the calling convention is based on which features are available? > (>128bit ext_vector_types are passed in AVX/AVX-512

[PATCH] D46135: [Driver, CodeGen] add options to enable/disable an FP cast optimization

scanon added a comment. I like Chandler's wording. Something like: "... this flag will attempt to cause " https://reviews.llvm.org/D46135 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34695: _Float16 preprocessor macro definitions

scanon accepted this revision. scanon added a comment. LGTM as well. https://reviews.llvm.org/D34695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34695: _Float16 preprocessor macro definitions

scanon added inline comments. Comment at: lib/Headers/float.h:137 +#ifdef __STDC_WANT_IEC_60559_TYPES_EXT__ +# define FLT16_MANT_DIG __FLT16_MANT_DIG__ rogfer01 wrote: > scanon wrote: > > rogfer01 wrote: > > > My understanding is that, given that we support

[PATCH] D34695: _Float16 preprocessor macro definitions

scanon added inline comments. Comment at: lib/Headers/float.h:137 +#ifdef __STDC_WANT_IEC_60559_TYPES_EXT__ +# define FLT16_MANT_DIG __FLT16_MANT_DIG__ rogfer01 wrote: > My understanding is that, given that we support TS18661-2 by default, this > macro

[PATCH] D28862: [compiler-rt] [test] Use approximate comparison on float types

scanon added a comment. These tests should either be exact, or should have a tolerance that's mathematically sound. +/-1ulp is not sound; the allowed error should be proportional to the magnitude of the larger of the real and imaginary components of the result -- e.g. if one component is very

[PATCH] D27898: [compiler-rt] [builtins] Implement __floattitf() & __floatuntitf()

scanon accepted this revision. scanon added a comment. This revision is now accepted and ready to land. OK, I'm satisfied with that. https://reviews.llvm.org/D27898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D27898: [compiler-rt] [builtins] Implement __floattitf() & __floatuntitf()

scanon added inline comments. Comment at: lib/builtins/floattitf.c:65 +if (a & ((tu_int)1 << LDBL_MANT_DIG)) { +a >>= 1; +++e; mgorny wrote: > scanon wrote: > > mgorny wrote: > > > scanon wrote: > > > > Strictly speaking there's

[PATCH] D27898: [compiler-rt] [builtins] Implement __floattitf() & __floatuntitf()

scanon requested changes to this revision. scanon added a reviewer: scanon. scanon added a comment. This revision now requires changes to proceed. I don't particularly care about the shift, since the code is completely equivalent either way, though it would be nice to clean up. However, please

[PATCH] D27898: [compiler-rt] [builtins] Implement __floattitf() & __floatuntitf()

scanon added inline comments. Comment at: lib/builtins/floatuntitf.c:73 +long_double_bits fb; +fb.u.high.all = (du_int)(e + 16383) << 48/* exponent */ + | ((a >> 64) & 0xLL); /* mantissa */ mgorny wrote: >

[PATCH] D27898: [compiler-rt] [builtins] Implement __floattitf() & __floatuntitf()

scanon added inline comments. Comment at: lib/builtins/floattitf.c:65 +if (a & ((tu_int)1 << LDBL_MANT_DIG)) { +a >>= 1; +++e; Strictly speaking there's no need to adjust `a` here. If we rounded up into a new binade, then `a` is

[PATCH] D27898: [compiler-rt] [builtins] Implement __floattitf() & __floatuntitf()

scanon added a comment. Please s/mantissa/significand/. I know that "mantissa" is used in a number of places in llvm sources, but it's incorrect terminology. Significand is the IEEE-754 nomenclature, which any new work should follow. https://reviews.llvm.org/D27898