[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

efriedma added inline comments. Comment at: clang/include/clang/Basic/AArch64SVETypeFlags.h:72 + bool isStructStore() const { return Flags & IsStructStore; } + bool isZxtReturn() const { return Flags & IsZxtReturn; } + SjoerdMeijer wrote: > nit: this one is non

[PATCH] D73687: [AArch64][SVE] Add SVE2 intrinsics for complex integer dot product

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73687/new/ https://reviews.llvm.org/D73687 ___ cfe-commits mailing list cfe-commi

[PATCH] D73543: [clang] Add support for __builtin_memcpy_inline

efriedma added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2252 + + * ``__builtin_memcpy_inline`` + This is in the wrong section of the documentation. We could constant-evaluate __builtin_memcpy_inline, I guess, but that isn't the primary pur

[PATCH] D73543: [clang] Add support for __builtin_memcpy_inline

efriedma added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1655 +TheCall->getArg(2)->isIntegerConstantExpr(I, Context); +if (I.isNullValue()) + break; You can EvaluateKnownConstInt here, instead of isIntegerConstantExpr. Repository

[PATCH] D73543: [clang] Add support for __builtin_memcpy_inline

efriedma added a comment. Sorry about the delayed response, I was traveling. Comment at: clang/docs/LanguageExtensions.rst:2252 + + * ``__builtin_memcpy_inline`` + gchatelet wrote: > efriedma wrote: > > This is in the wrong section of the documentation. We cou

[PATCH] D73543: [clang] Add support for __builtin_memcpy_inline

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73543/new/ https://reviews.llvm.org/D73543 ___

[PATCH] D71600: PowerPC 32-bit - forces 8 byte lock/lock_free decisions at compiled time

efriedma requested changes to this revision. efriedma added a comment. This revision now requires changes to proceed. For the clang change, we should do something like D72579 , not explicitly check for a specific target in target-independent code. For compiler-r

[PATCH] D71600: PowerPC 32-bit - forces 8 byte lock/lock_free decisions at compiled time

efriedma added a comment. In D71600#1867320 , @adalava wrote: > > For compiler-rt, are you really disabling > > COMPILER_RT_EXCLUDE_ATOMIC_BUILTIN? Are you sure you understand the > > implications of that? > > I didn't understood "disable COMPILER_RT_E

[PATCH] D71600: PowerPC 32-bit - forces 8 byte lock/lock_free decisions at compiled time

efriedma added a comment. atomic.c is disabled by default for a good reason: it doesn't work correctly in general. In particular, if an atomic variable is shared across two shared libraries, the two libraries will use different locks, and therefore the operation won't be atomic. It might make

[PATCH] D74386: [SVE] Update API ConstantVector::getSplat() to use ElementCount.

efriedma added a comment. This is sort of a large number of functional changes, but it's probably okay to skip test coverage for all the simple getNumElements->getElementCount changes; it's obvious it can't break anything. Comment at: llvm/include/llvm/Analysis/Utils/Local.h:

[PATCH] D71037: [Diagnostic] Add ftabstop to -Wmisleading-indentation

efriedma added a comment. I just ran into this warning, and I think there's a bit of a discoverability problem related to the width of tabs and -ftabstop. If you have mixed tabs and spaces, and you've correctly specified the tab stop width with -ftabstop, everything works fine. If you haven't

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

efriedma added a comment. > https://gcc.godbolt.org/z/cY9-HQ gcc's behavior for your testcase makes no sense. We have to emit the definition of a static function: it can't be defined in any other translation unit because it's impossible to name in any other translation unit. Note the "_ZL" p

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

efriedma added a comment. I agree, it doesn't make sense to warn on static functions; the behavior didn't change, and there's only one reasonable result. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67414/new/ https://reviews.llvm.org/D67414 _

[PATCH] D74724: [AArch64][SVE] CodeGen of ACLE Builtin Types

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:559 #include "clang/Basic/AArch64SVEACLETypes.def" -{ - unsigned DiagID = CGM.getDiags().getCustomDiagID( - DiagnosticsEngine::Error, - "cannot yet generate code for SVE typ

[PATCH] D74769: [AArch64][SVE] Add SVE2 intrinsics for polynomial arithmetic

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74769/new/ https://reviews.llvm.org/D74769 ___

[PATCH] D74724: [AArch64][SVE] CodeGen of ACLE Builtin Types

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74724/new/ https://reviews.llvm.org/D74724 ___ cfe-commits mailing list cfe-commi

[PATCH] D72825: Remove unused option that gcc ignored

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72825/new/ https://reviews.llvm.org/D72825 ___

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. Seems fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74801/new/ https://reviews.llvm.org/D74801

[PATCH] D72869: Add __warn_memset_zero_len builtin as a workaround for glibc issue

efriedma added a comment. This is very hacky, but it might be the least-bad alternative. I mean, we could change D71082 so it doesn't allow system headers to define memset, but that seems worse. Please add a testcase. Repository: rG LLVM Github Monorepo C

[PATCH] D72869: Add __warn_memset_zero_len builtin as a workaround for glibc issue

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72869/new/ https://reviews.llvm.org/D72869 ___

[PATCH] D71469: [AArch64] Add sq(r)dmulh_lane(q) LLVM IR intrinsics

efriedma added a comment. > This makes it impossible to do a neat trick when using NEON intrinsics: one > can load a number of constants using a single vector load, which are then > repeatedly used to multiply whole vectors by one of the constants. This trick > is used for a nice performance up

[PATCH] D72869: Add __warn_memset_zero_len builtin as a workaround for glibc issue

efriedma added a comment. We currently don't have any mechanism for restricting builtins to specific operating systems. I guess we could add one, but this change doesn't seem like a compelling argument to add that capability. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D73097: [AArch64][SVE] Add intrinsics for FFR manipulation

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM I think it's fairly likely that we'll eventually decide that the correct model of the FFR register doesn't require these intrinsics at all. They're just register copies, which is no

[PATCH] D72434: Support offset of member designator with the arrow for __builtin_offsetof

efriedma added a comment. Testcase? I assume we give appropriate errors if you try to use "->" on a member that's not an array? Comment at: clang/lib/Parse/ParseExpr.cpp:2168 -// FIXME: This loop leaks the index expressions on error. while (1) { Di

[PATCH] D72467: Remove "mask" operand from shufflevector.

efriedma marked 5 inline comments as done. efriedma added inline comments. Comment at: llvm/include/llvm/IR/IRBuilder.h:2551 +SmallVector IntMask; +IntMask.assign(Mask.begin(), Mask.end()); +return CreateShuffleVector(V1, V2, IntMask, Name); ctetreau

[PATCH] D72467: Remove "mask" operand from shufflevector.

efriedma updated this revision to Diff 240071. efriedma added a comment. Addressed review comments. Rebased. Chris Tetreault mentioned offline that we might want to change the return value of `getShuffleMask()` now, to try to save some work later when we add more forms of scalable vector shuff

[PATCH] D72467: Remove "mask" operand from shufflevector.

efriedma marked 5 inline comments as done. efriedma added a comment. In D72467#1838591 , @spatel wrote: > LGTM, but should get a 2nd opinion since I'm not familiar with some of the > parts. Any specific part you're worried about? Com

[PATCH] D73495: [CodeGen] Attach no-builtin attributes to functions with no Decl

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:1917 const auto *NBA = Fn->getAttr(); -bool HasWildcard = NBA && llvm::is_contained(NBA->builtinNames(), "*"); -if (getLangOpts().NoBuiltin || HasWildcard) - FuncAttrs.addAt

[PATCH] D73493: [AArch64][SVE] Add SVE2 intrinsics for uniform DSP operations

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73493/new/ https://reviews.llvm.org/D73493 ___

[PATCH] D73495: [CodeGen] Attach no-builtin attributes to function definitions with no Decl

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:1917 const auto *NBA = Fn->getAttr(); -bool HasWildcard = NBA && llvm::is_contained(NBA->builtinNames(), "*"); -if (getLangOpts().NoBuiltin || HasWildcard) - FuncAttrs.addAt

[PATCH] D71469: [AArch64] Add IR intrinsics for sq(r)dmulh_lane(q)

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71469/new/ https://reviews.llvm.org/D71469 ___

[PATCH] D73495: [CodeGen] Attach no-builtin attributes to function definitions with no Decl

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM There's maybe some argument that we should be calling getNonClosureContext() or something like that to find the parent function, at least for some attributes. But that seems less cr

[PATCH] D72467: Remove "mask" operand from shufflevector.

efriedma updated this revision to Diff 241333. efriedma added a comment. Address review comments, rebase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72467/new/ https://reviews.llvm.org/D72467 Files: clang/lib/CodeGen/CGExpr.cpp llvm/include

[PATCH] D73687: [AArch64][SVE] Add SVE2 intrinsics for complex integer dot product

efriedma added inline comments. Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td: + LLVMSubdivide4VectorType<0>, + llvm_i32_ty], +[IntrNoMem]>; Missing ImmArg? Comment at: llvm/include/ll

[PATCH] D73543: [clang] Add support for __builtin_memcpy_inline

efriedma added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1653 +llvm::APSInt I(64); +if (!TheCall->getArg(2)->isIntegerConstantExpr(I, Context)) + Diag(TheCall->getBeginLoc(), diag::err_expr_not_ice) SemaBuiltinConstantArg. Or actu

[PATCH] D44706: Delete BuiltinCC

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D44706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

efriedma added inline comments. Comment at: test/Sema/ms-forceinline-on-variadic.cpp:14 +__builtin_va_end(ap); +} + compnerd wrote: > DHowett-MSFT wrote: > > compnerd wrote: > > > Would be nice to have a second test that uses the Microsoft definitions > > >

[PATCH] D44844: [PR36880] Avoid -Wunused-lambda-capture false positive for explicit this capture used in an unresolved member in a generic lambda

efriedma added inline comments. Comment at: lib/Sema/TreeTransform.h:11269 +// 'this' capture is marked as 'used'. +if (Old->isImplicitCXXThisAccess()) + getSema().CheckCXXThisCapture(Old->getMemberNameInfo().getLoc(), This doesn't make sense; in gen

[PATCH] D44727: [RISCV] Extend getTargetDefines for RISCVTargetInfo

efriedma added a comment. Do the macros you're defining here match gcc? Comment at: lib/Basic/Targets/RISCV.cpp:68 + +bool RISCVTargetInfo::hasFeature(StringRef Feature) const { + return llvm::StringSwitch(Feature) asb wrote: > It seems a number of other targe

[PATCH] D45109: Remove -cc1 option "-backend-option"

efriedma created this revision. Herald added subscribers: eraman, javed.absar, mehdi_amini. It means essentially the same thing as -mllvm; there isn't any reason to have separate options. Repository: rC Clang https://reviews.llvm.org/D45109 Files: include/clang/Driver/CC1Options.td incl

[PATCH] D41316: [libcxx] Allow random_device to be built optionally

efriedma added a comment. Yes, the standard says you're allowed to throw an exception from the random_device constructor, or use a PRNG with an arbitrary seed, or even just return zeros from operator(). But none of those behaviors are actually useful; the code will compile, but you won't get th

[PATCH] D45196: [libc++abi] Replace __sync_* functions with __libcpp_atomic_* functions.

efriedma created this revision. efriedma added reviewers: EricWF, compnerd. This is basically part 2 of r313694. It's a little unfortunate that I had to copy-paste atomic_support.h, but I don't really see any alternative. The refstring.h changes are the same as the libcxx changes in r313694.

[PATCH] D45240: [ARM] Compute a target feature which corresponds to the ARM version.

efriedma created this revision. efriedma added reviewers: compnerd, SjoerdMeijer, fhahn. Herald added subscribers: kristof.beyls, javed.absar, mehdi_amini. Currently, the interaction between the triple, the CPU, and the supported features is a mess: the driver edits the triple to indicate the sup

[PATCH] D45240: [ARM] Compute a target feature which corresponds to the ARM version.

efriedma updated this revision to Diff 141078. efriedma added a comment. Fixed to use the arch name as the "feature"; otherwise, we miss relevant features in certain cases. This is taken from ARMAsmParser::parseDirectiveArch. Repository: rC Clang https://reviews.llvm.org/D45240 Files: li

[PATCH] D45240: [ARM] Compute a target feature which corresponds to the ARM version.

efriedma added inline comments. Comment at: lib/Basic/Targets/ARM.cpp:345 // get default FPU features + llvm::ARM::ArchKind Arch = llvm::ARM::parseArch(getTriple().getArchName()); unsigned FPUKind = llvm::ARM::getDefaultFPU(CPU, Arch); fhahn wrote: > Is th

[PATCH] D45240: [ARM] Compute a target feature which corresponds to the ARM version.

efriedma updated this revision to Diff 141237. efriedma added a comment. Compute arch used passed-in CPU, not the global arch. Repository: rC Clang https://reviews.llvm.org/D45240 Files: lib/Basic/Targets/ARM.cpp test/CodeGen/arm-long-calls.c test/CodeGen/arm-no-movt.c test/CodeGen/a

[PATCH] D41316: [libcxx] Allow random_device to be built optionally

efriedma updated this revision to Diff 141255. efriedma added a comment. Get rid of the test changes. They were broken in multiple ways, and weren't really useful anyway because the targets where you would want to use this can't run the libcxx testsuite anyway (because they don't have an operat

[PATCH] D45240: [ARM] Compute a target feature which corresponds to the ARM version.

efriedma updated this revision to Diff 141393. efriedma added a comment. Add a bunch of tests for various arm arches. Make sure we do something reasonable when we can't figure out any arch at all. Repository: rC Clang https://reviews.llvm.org/D45240 Files: lib/Basic/Targets/ARM.cpp tes

[PATCH] D45240: [ARM] Compute a target feature which corresponds to the ARM version.

efriedma added a comment. > I take it we still have test cases for the arm <-> thumb transition? "-mthumb" and "-marm" are handled in the driver by rewriting the triple. Repository: rC Clang https://reviews.llvm.org/D45240 ___ cfe-commits mailin

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

efriedma added a comment. Rather than adding weird hacks to merging, can we just reject any attempt to redeclare a builtin? Repository: rC Clang https://reviews.llvm.org/D45383 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

efriedma added a comment. By "builtins" I meant specifically the stuff that that user code isn't allowed to declare, like __builtin_*, since they often have weird/incomplete signatures. Repository: rC Clang https://reviews.llvm.org/D45383 ___ cf

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

efriedma added a comment. How could this patch possibly affect vprintf? Repository: rC Clang https://reviews.llvm.org/D45383 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

efriedma added a comment. Identical to what? `__builtin_va_start` and `__builtin_va_end` specifically are weird because they're builtins which have a signature which can't be expressed in C. vprintf doesn't have that problem. Builtins.def makes the relevant distinction already: a "BUILTIN" is

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

efriedma added a comment. > vprintf is handled using the exact same code-paths. SO, it'll have its 2nd > parameter created with type 'char*&' vprintf is defined in the C standard as "int vprintf(const char *format, va_list arg);"; on Windows, that's equivalent to "int vprintf(const char *forma

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

efriedma added a comment. > I don't see redeclaring the latter 3 as being disallowed, It is in fact disallowed by the standard, in the section describing va_start/va_arg/va_end/va_copy: "If a macro definition is suppressed in order to access an actual function, or a program defines an external

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

efriedma added a comment. > I believe it is permissible to implement va_copy and va_end as a function, > isn't it? I guess you could, in theory, but LLVM doesn't support any targets which do that. Repository: rC Clang https://reviews.llvm.org/D45383 _

[PATCH] D45383: Strip reference from a va_list object in C when merging parameter types.

efriedma added a comment. Basically, yes, although I was thinking you could be a bit more aggressive. At least, anything marked "t" for custom typechecking is probably not something which should be redeclared. Repository: rC Clang https://reviews.llvm.org/D45383

[PATCH] D45109: Remove -cc1 option "-backend-option"

efriedma added reviewers: pcc, echristo, rsmith. efriedma added a comment. Not sure who to add as reviewers here. Repository: rC Clang https://reviews.llvm.org/D45109 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D45196: [libc++abi] Replace __sync_* functions with __libcpp_atomic_* functions.

efriedma added a comment. Ping. Repository: rCXXA libc++abi https://reviews.llvm.org/D45196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45383: Limit types of builtins that can be redeclared.

efriedma added inline comments. Comment at: test/Sema/builtin-redecl.cpp:9 +// Overloading a builtin is acceptable in C++. +void __builtin_va_copy(double d); +#endif We don't want to allow this; in particular, for builtins with custom type-checking, we have no w

[PATCH] D43578: -ftime-report switch support in Clang

efriedma added a comment. > while both git and svn have revert subcommands This is kind of misleading; yes, both git and svn have subcommands named "revert", but "svn revert" doesn't have the right meaning. You have to use "svn merge" to revert a committed change. Repository: rL LLVM http

[PATCH] D39321: ARM: centralise SizeType, PtrDiffType, and IntPtrType

efriedma added a comment. I think you missed a couple other places which still set SizeType and PtrDiffType? Repository: rL LLVM https://reviews.llvm.org/D39321 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bi

[PATCH] D33765: Show correct column nr. when multi-byte utf8 chars are used.

efriedma added a comment. I didn't really search for it before, but it looks like LLVM already has a routine for computing column widths? See llvm::sys::unicode::columnWidthUTF8. There are some tools which parse clang diagnostic output; we might need a flag to control this. Not sure who would

[PATCH] D39321: ARM: centralise SizeType, PtrDiffType, and IntPtrType

efriedma added a comment. Are you sure the change to APCS is right? I mean, it looks like it's right if I compare to gcc with -mabi=gnu-apcs, but I'm not sure what, exactly, we're trying to be compatible with, so I'd prefer not to touch it, especially not in a patch with a bunch of changes whi

[PATCH] D39321: ARM: centralise SizeType, PtrDiffType, and IntPtrType

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. I guess... fine. LGTM, assuming we have test coverage for all the different cases. Repository: rL LLVM https://reviews.llvm.org/D39321 __

[PATCH] D39204: [CodeGen] __builtin_sqrt should map to the compiler's intrinsic sqrt function

efriedma added a comment. > I was going to include the builtins too, but that exposes another bug (marked > here with FIXME) - the builtins are all defined 'const'. Probably just need to change c->e in Builtins.def? > This does make me curious about the use-case of libm-equivalent builtins. If

[PATCH] D39204: [CodeGen] __builtin_sqrt should map to the compiler's intrinsic sqrt function

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. Let's do this one step at a time; first this patch, then fix the __builtin_* functions to use "e", then add all the missing cases CodeGenFunction::EmitBuiltinExpr. This patch LGTM, assumi

[PATCH] D39481: [CodeGen] fix const-ness of builtin equivalents of and functions that might set errno

efriedma added a comment. Please make these consistent with the non-__builtin versions. Granted, I'm not sure all of those are right... I'm pretty sure, for example, cbrt can't set errno, and carg can. But I'd prefer to deal with that in a separate patch. https://reviews.llvm.org/D39481 _

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

efriedma added a comment. > The internal canonical types are compared, so all this typedef sugar will be > silently ignored. Yes, and that's precisely the problem. On many 32-bit platforms, both "size_t" and "uint32_t" are typedefs for "unsigned int"; on some 32-bit platforms, "size_t" is an

[PATCH] D39481: [CodeGen] fix const-ness of builtin equivalents of and functions that might set errno

efriedma added a comment. I think we should assume libm complies with the C99 standard (specifically, the appendix specifying the behavior of IEEE floating-point). Otherwise, we have no basis to predict the behavior of any standard library call. From the standard: "Functions with a NaN argumen

[PATCH] D39481: [CodeGen] fix const-ness of builtin equivalents of and functions that might set errno

efriedma added a comment. > But we don't check the const attribute in CGBuiltin.cpp before converting it > to an intrinsic In practice, fma() implementations don't set errno (at least, glibc and msvcrt don't). And people would be really annoyed if we forced them to use fast-math flags to gene

[PATCH] D39481: [CodeGen] fix const-ness of builtin equivalents of and functions that might set errno

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM. (For reference, lgamma is weird because of the POSIX signgam.) https://reviews.llvm.org/D39481 ___ cfe-commits mailing list cfe-commit

[PATCH] D39611: [CodeGen] make cbrt and fma constant (never set errno); document complex calls as always constant

efriedma added a comment. > The POSIX docs all have language like this for complex calls: > "No errors are defined." I would not trust the POSIX documentation. carg() is an alias for atan2(), and cabs() is an alias for hypot(), so they should be marked the same way. (On glibc, cabs/hypot will

[PATCH] D39611: [CodeGen] make cbrt and fma constant (never set errno); document complex calls as always constant

efriedma added inline comments. Comment at: include/clang/Basic/Builtins.def:169 +// Disregard that 'cbrt' could set errno with a NaN input. The C standard says +// that NaN arguments generally do not raise FP exceptions. +BUILTIN(__builtin_cbrt , "dd", "Fnc") No

[PATCH] D39615: [CodeGen] add remquo to list of recognized library calls

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D39615 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/c

[PATCH] D39611: [CodeGen] make cbrt and fma constant (never set errno); document complex calls as always constant

efriedma added a comment. > In general, because all of these functions are well defined over the entire > complex domain, they don't have errors. The "no errors defined" is likely > correct. One, it is not true that all these functions are well-defined over the entire complex domain. For exam

[PATCH] D39641: [CodeGen] make cbrt and fma constant (never set errno)

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM, but wait a couple days before you commit to see if anyone else wants to comment. https://reviews.llvm.org/D39641 ___ cfe-commits maili

[PATCH] D39611: [CodeGen] make cbrt and fma constant (never set errno); document complex calls as always constant

efriedma added a comment. Currently, the clang driver supports three platforms which have errno-setting libc implementations: GNU/Linux (with glibc), Solaris, and Windows (with Visual Studio CRT). (BSD-based systems, including OS X, never set errno in libm, so they aren't relevant here.) As l

[PATCH] D39627: Fix vtable not receiving hidden visibility when using push(visibility)

efriedma added subscribers: cfe-commits, efriedma. efriedma added a comment. Adding cfe-commits. (In the future, please make sure to CC that list instead of llvm-commits for clang changes.) > I'm missing some background lingo here. What is the meaning of "a > declaration" and "a definition" he

[PATCH] D39641: [CodeGen] make cbrt and fma constant (never set errno)

efriedma added a comment. cbrt() can't underflow; that's not how cube roots work. If 0 x. See also the definition of rootn() in IEEE 754 (which specifies no exceptions for n=3). For fma(), sure, I guess we could whitelist glibc and msvcrt, if you're worried there's some unknown implementatio

[PATCH] D39641: [CodeGen] make cbrt and fma constant (never set errno)

efriedma added inline comments. Comment at: lib/Sema/SemaDecl.cpp:12853 + const llvm::Triple &Trip = Context.getTargetInfo().getTriple(); + if ((Trip.isGNUEnvironment() || Trip.isKnownWindowsMSVCEnvironment()) && + (Name->isStr("fma") || Name->isStr("fmaf") ||

[PATCH] D39641: [CodeGen] make cbrt and fma constant (never set errno)

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

[PATCH] D40023: [RISCV] Implement ABI lowering

efriedma added reviewers: efriedma, rjmccall. efriedma added a comment. You need more test coverage for the cases where arguments end up on the stack. And some test coverage for varargs calls. Comment at: lib/CodeGen/TargetInfo.cpp:8858 + else +NeededArgGPRs = 1; + -

[PATCH] D40023: [RISCV] Implement ABI lowering

efriedma added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:8913 + } + return getNaturalAlignIndirect(Ty, /*ByVal=*/true); +} The spec says "Aggregates larger than 2✕XLEN bits are passed by reference and are replaced in the argument list with the ad

[PATCH] D40144: Implement `std::launder`

efriedma added inline comments. Comment at: include/new:174 +_LIBCPP_NODISCARD_AFTER_CXX17 inline _LIBCPP_INLINE_VISIBILITY +constexpr _Tp* launder(_Tp* __p) noexcept { return __p;} +#endif How is the compiler supposed to know that "std::__1::launder()" has speci

[PATCH] D40144: Implement `std::launder`

efriedma added inline comments. Comment at: include/new:174 +_LIBCPP_NODISCARD_AFTER_CXX17 inline _LIBCPP_INLINE_VISIBILITY +constexpr _Tp* launder(_Tp* __p) noexcept { return __p;} +#endif efriedma wrote: > How is the compiler supposed to know that "std::__1::la

[PATCH] D39611: [CodeGen] change const-ness of complex calls

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D39611 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/c

[PATCH] D40256: [ARM] disable FPU features when using soft floating point.

efriedma added a comment. -mfpu controls what floating-point/vector instructions the compiler generates. -mfloat-abi controls whether floating-point arguments to functions are passed in floating-point registers. These are completely independent, and we need to support using both of them toget

[PATCH] D40256: [ARM] disable FPU features when using soft floating point.

efriedma added a comment. Oh, I see, for some silly reason there are actually *three* -mfloat-abi options: hard, soft, and softfp. hard means float instructions and a hard-float calling convention, soft means no floating-point and a soft-float convention, and softfp means float instructions an

[PATCH] D145369: Emit const globals with constexpr destructor as constant LLVM values

efriedma added inline comments. Comment at: clang/test/CodeGenCXX/static-init.cpp:181 +#if __cplusplus >= 202002L +// A const object with constexpr destructor can be emitted as a constant. +namespace test5 { I don't see how this works. For a static local variabl

[PATCH] D145369: Emit const globals with constexpr destructor as constant LLVM values

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4344 + (Record->hasTrivialDestructor() || + Record->hasConstexprDestructor()); } For the purposes of CodeGen, checking `Record->hasConstexprDestructor()` i

[PATCH] D145564: [clang][docs] Clarify the semantics of -fexceptions

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145564/new/ https://reviews.llvm.org/D145564 ___ cfe-commits mailing list cfe-commi

[PATCH] D145150: clang: Emit nofpclass(nan inf) for -ffinite-math-only

efriedma added a comment. Looks fine from a codegen perspective, assuming these are the semantics we want for -ffinite-math-only. Comment at: clang/lib/CodeGen/CGCall.cpp:3052 +AI->addAttrs( +llvm::AttrBuilder(getLLVMContext()).addNoFPClassAttr(Mask)); +

[PATCH] D145150: clang: Emit nofpclass(nan inf) for -ffinite-math-only

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145150/new/ https://reviews.llvm.org/D145150 ___ cfe-commits mailing list cfe-commi

[PATCH] D145369: Emit const globals with constexpr destructor as constant LLVM values

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/CodeGen/CGExprAgg.cpp:535 CGM.getModule(), C->getType(), - CGM.isTypeConstant(ArrayQTy, /* ExcludeCtorDtor= */ true), +

[PATCH] D146211: Make globals used for array initialization codegen constant

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146211/new/ https://reviews.llvm.org/D146211 ___

[PATCH] D146370: [Clang][OpenMP]Solved the the always truth condition in Arm64

efriedma added a comment. The patch is currently impossible to read; please don't clang-format the whole file. There's a script clang-format-diff.py that can format just the parts of a file you change, if you need it. As noted in the discussion on the bug report, needs a testcase. Repository

[PATCH] D146386: [MS ABI] Fix mangling references to declarations.

efriedma added inline comments. Comment at: clang/lib/AST/MicrosoftMangle.cpp:720 +const CXXRecordDecl *RD, const ValueDecl *VD) { + MSInheritanceModel IM = RD->getMSInheritanceModel(); + // ::= It's not obvious to me why the inheritance model is relevant

[PATCH] D146466: [clang] diagnose function fallthrough

efriedma added a comment. I'm concerned about the potential for false positives: - If a user doesn't mark a function noreturn, but it doesn't actually ever return. - A function is conditionally broken, but the condition is never actually true (for example, jump threading creates a dead codepath

[PATCH] D146466: [clang] diagnose function fallthrough

efriedma added a comment. > Yes, but kernel developers would prefer to diagnose issues at compile time > when possible, rather that at runtime. Function fallthough is diagnosable at > compile time, as this patch demonstrates. I'm not saying the diagnostic is useless. I'm saying users are going

<    10   11   12   13   14   15   16   17   18   >