[PATCH] D43430: Omit nullptr check for sufficiently simple delete-expressions
ahh created this revision. ahh added a reviewer: rsmith. Herald added a subscriber: cfe-commits. [expr.delete] is pretty crystal clear that it's OK to invoke a deallocation-function on a nullptr delete-expression: "If the value of the operand of the delete-expression is a null pointer value, it is unspecified whether a deallocation function will be called as described above." Even so, we currently check against nullptr unconditionally. This is wasteful for anything with a trivial destructor; deleting nullptr is very rare so it's not worth saving the call to ::operator delete, and this is a useless branch (and waste of code size) in the common case. Condition emission of the branch on us needing to actually look through the pointer for a vtable, size cookie, or nontrivial destructor. (In principle a nontrivial destructor with no side effects that didn't touch the object would also be safe to run unconditionally, but I don't know how to test we have one of those and who in the world would write one?) On an important and very large (~500 MiB) Google search binary, this saves approximately 32 KiB of text. Okay, it's not impressive in a relative sense, but it's still the right thing to do. A note on optimization: we still successfully elide delete-expressions of (literal) nullptr. Where before they were stuck behind a never-taken branch, now they reduce (effectively) to calls to __builtin_operator_delete(nullptr), which EarlyCSE is capable of optimizing out. So this has no cost in the already well-optimized case. Repository: rC Clang https://reviews.llvm.org/D43430 Files: lib/CodeGen/CGCXXABI.h lib/CodeGen/CGExprCXX.cpp test/CodeGenCXX/cxx2a-destroying-delete.cpp test/CodeGenCXX/delete-two-arg.cpp test/CodeGenCXX/delete.cpp Index: test/CodeGenCXX/delete.cpp === --- test/CodeGenCXX/delete.cpp +++ test/CodeGenCXX/delete.cpp @@ -9,7 +9,12 @@ }; // POD types. +// CHECK-LABEL: define void @_Z2t3P1S void t3(S *s) { + // CHECK-NOT: icmp eq {{.*}}, null + // CHECK-NOT: br i1 + // CHECK: call void @_ZdlPv + delete s; } @@ -99,6 +104,8 @@ namespace test3 { void f(int a[10][20]) { +// CHECK-NOT: icmp eq {{.*}}, null +// CHECK-NOT: br i1 // CHECK: call void @_ZdaPv(i8* delete a; } @@ -113,6 +120,8 @@ // CHECK-LABEL: define void @_ZN5test421global_delete_virtualEPNS_1XE void global_delete_virtual(X *xp) { +// CHECK: icmp eq {{.*}}, null +// CHECK: br i1 // Load the offset-to-top from the vtable and apply it. // This has to be done first because the dtor can mess it up. // CHECK: [[T0:%.*]] = bitcast [[X:%.*]]* [[XP:%.*]] to i64** Index: test/CodeGenCXX/delete-two-arg.cpp === --- test/CodeGenCXX/delete-two-arg.cpp +++ test/CodeGenCXX/delete-two-arg.cpp @@ -9,8 +9,8 @@ // CHECK-LABEL: define void @_ZN5test11aEPNS_1AE( void a(A *x) { // CHECK: load -// CHECK-NEXT: icmp eq {{.*}}, null -// CHECK-NEXT: br i1 +// CHECK-NOT: icmp eq {{.*}}, null +// CHECK-NOT: br i1 // CHECK: call void @_ZN5test11AdlEPvj(i8* %{{.*}}, i32 4) delete x; } Index: test/CodeGenCXX/cxx2a-destroying-delete.cpp === --- test/CodeGenCXX/cxx2a-destroying-delete.cpp +++ test/CodeGenCXX/cxx2a-destroying-delete.cpp @@ -15,9 +15,8 @@ void delete_A(A *a) { delete a; } // CHECK-LABEL: define {{.*}}delete_A // CHECK: %[[a:.*]] = load -// CHECK: icmp eq %{{.*}} %[[a]], null -// CHECK: br i1 -// +// CHECK-NOT: icmp eq %{{.*}} %[[a]], null +// CHECK-NOT: br i1 // Ensure that we call the destroying delete and not the destructor. // CHECK-NOT: call // CHECK-ITANIUM: call void @_ZN1AdlEPS_St19destroying_delete_t(%{{.*}}* %[[a]]) @@ -60,8 +59,8 @@ // CHECK: %[[castbase:.*]] = bitcast {{.*}} %[[base]] // // CHECK: %[[a:.*]] = phi {{.*}} %[[castbase]] -// CHECK: icmp eq %{{.*}} %[[a]], null -// CHECK: br i1 +// CHECK-NOT: icmp eq %{{.*}} %[[a]], null +// CHECK-NOT: br i1 // // CHECK-NOT: call // CHECK-ITANIUM: call void @_ZN1AdlEPS_St19destroying_delete_t(%{{.*}}* %[[a]]) Index: lib/CodeGen/CGExprCXX.cpp === --- lib/CodeGen/CGExprCXX.cpp +++ lib/CodeGen/CGExprCXX.cpp @@ -1971,26 +1971,58 @@ CGF.PopCleanupBlock(); } -void CodeGenFunction::EmitCXXDeleteExpr(const CXXDeleteExpr *E) { - const Expr *Arg = E->getArgument(); - Address Ptr = EmitPointerWithAlignment(Arg); +// Will we read through the deleted pointer? If so, +// we must first check it is not null. +static bool DeleteMightAccessObject(CodeGenFunction &CGF, + const CXXDeleteExpr *E, + QualType DeleteTy) { - // Null check the pointer. - llvm::BasicBlock *DeleteNotNull = createBasicBlock("delete.notnull"); - llvm::BasicBlock *
[PATCH] D43430: Omit nullptr check for sufficiently simple delete-expressions
ahh added a comment. On my workstation's checkout of head, one test fails (Clang :: Driver/response-file.c) both with and without this change; everything else appears to pass. I believe that between the tests I add to delete.cpp and the ones that are already there (and destroying-delete.cpp) we cover every case that has to get a nullptr check, and pretty much every one that should *not*. Name of the helper function is, uh, good enough for me, but no objections to changing it. Repository: rC Clang https://reviews.llvm.org/D43430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r325391 - [OPENMP] Do not emit messages for templates in declare target
Hi Alexey, I think that's mostly my test case from PR35348? Am 2018-02-16 22:23, schrieb Alexey Bataev via cfe-commits: Author: abataev Date: Fri Feb 16 13:23:23 2018 New Revision: 325391 URL: http://llvm.org/viewvc/llvm-project?rev=325391&view=rev Log: [OPENMP] Do not emit messages for templates in declare target constructs. The compiler may emit some extra warnings for functions, that are implicit specialization of the templates, declared in the target region. Modified: cfe/trunk/lib/Sema/SemaOpenMP.cpp cfe/trunk/test/OpenMP/declare_target_messages.cpp [...] Modified: cfe/trunk/test/OpenMP/declare_target_messages.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/declare_target_messages.cpp?rev=325391&r1=325390&r2=325391&view=diff == --- cfe/trunk/test/OpenMP/declare_target_messages.cpp (original) +++ cfe/trunk/test/OpenMP/declare_target_messages.cpp Fri Feb 16 13:23:23 2018 @@ -33,6 +33,33 @@ struct NonT { typedef int sint; +template +T bla1() { return 0; } + +#pragma omp declare target +template +T bla2() { return 0; } +#pragma omp end declare target + +template<> +float bla2() { return 1.0; } + +#pragma omp declare target +void blub2() { + bla2(); I don't agree with this: The compiler has to warn about calling an explicit template specialization that is outside of any 'declare target' region. That's at least the case for OpenMP 4.5, I know that there are changes for OpenMP 5.0. But in that case the compiler needs to add an implicit 'declare target' attribute to generate correct code. ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43197: [OpenMP] Add flag for linking runtime bitcode library
Hahnfeld added a comment. Looking more closely at the patch, this doesn't seem to look into the `lib` / `lib64` next to the compiler. I'm not sure if `LIBRARY_PATH` is set for every installation, so I think we should add this one to catch the obvious case. This probably needs some attention for the tests because they'll find the just-built libraries... Comment at: lib/Driver/ToolChains/Cuda.cpp:536-542 + StringRef CompilerPath = env; + while (!CompilerPath.empty()) { +std::pair Split = +CompilerPath.split(llvm::sys::EnvPathSeparator); +LibraryPaths.push_back(Split.first); +CompilerPath = Split.second; + } `tools::addDirectoryList` uses `StringRef::find`, I'm not sure if `StringRef::split` creates real copies of the string... Repository: rC Clang https://reviews.llvm.org/D43197 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42800: Let CUDA toolchain support amdgpu target
Hahnfeld added a comment. Let's start with the obvious points: 1. The latest patch clearly wasn't run through `clang-format`. 2. It only has some 90 lines of context which makes it look like you deleted quite some code when browsing through the history. 3. This patch is still large, did you at least consider splitting it as I proposed some weeks ago? Additionally I don't see responses to all points that @tra raised. Their answers will probably explain the approach you chose, so I think they should also be added to the summary. Comment at: lib/Basic/Cuda.cpp:290-302 + case CudaArch::GFX701: + case CudaArch::GFX702: + case CudaArch::GFX703: + case CudaArch::GFX704: + case CudaArch::GFX800: + case CudaArch::GFX801: + case CudaArch::GFX802: This means you can't compile when the Clang driver detected an installation of CUDA 9? Comment at: lib/Basic/Targets/AMDGPU.h:383 uint64_t getNullPointerValue(LangAS AS) const override { +if(getTriple().getOS()==llvm::Triple::CUDA) return 0; return AS == LangAS::opencl_local ? ~0 : 0; How can `LangAS::opencl_local` ever be used in CUDA? I think this check is redundant. Comment at: lib/Driver/Driver.cpp:3262-3263 !C.getArgs().hasArg(options::OPT_traditional_cpp) && !SaveTemps && + (InputType != types::TY_LLVM_IR) && + (InputType != types::TY_LLVM_BC) && !C.getArgs().hasArg(options::OPT_rewrite_objc); I'm not sure I understand this change to the generic driver code: How can LLVM IR / BC ever need a preprocessor? Comment at: lib/Driver/Driver.cpp:3954-3957 +if (((StringRef)BaseInput).endswith(".a")) + Suffixed += "a"; +else + Suffixed += Suffix; If this is really needed this deserves some justification. Comment at: lib/Driver/ToolChains/Cuda.cpp:359-361 + addBCLib(C, Args, CmdArgs, LibraryPaths, "opencl.amdgcn.bc"); + addBCLib(C, Args, CmdArgs, LibraryPaths, "ockl.amdgcn.bc"); + addBCLib(C, Args, CmdArgs, LibraryPaths, "irif.amdgcn.bc"); gregrodgers wrote: > arsenm wrote: > > Why is this done under an NVPTX:: class > Because we are not creating a new toolchain for AMDGCN. We modify the logic > in the tool constructor as needed for AMDGCN, keeping the ability to provide > a set of mixed targets. That sounds more like a hack, the CUDA classes should be separated from NVPTX and AMDGCN. Comment at: lib/Driver/ToolChains/Cuda.cpp:639-640 +CmdArgs2.push_back(Args.MakeArgString(Output.getFilename())); +const char *Exec2 = +Args.MakeArgString(C.getDriver().Dir + "/clang-fixup-fatbin"); +C.addCommand( gregrodgers wrote: > Hahnfeld wrote: > > `clang-fixup-fatbin` is not upstreamed and won't work. Sounds like a > > horrible name btw... > Major cleanup here in the next update. It is not a bad name when you see the > update and the comments in the update. I wasn't only criticizing the name but also the fact that this code won't work with only upstream components! Comment at: lib/Driver/ToolChains/Cuda.cpp:293 + if (useOpenHeaders()) { +CC1Args.push_back("__clang_cuda_runtime_wrapper_open.h"); +CC1Args.push_back("-D__USE_OPEN_HEADERS__"); I don't see this file in the upstream repository? Comment at: lib/Driver/ToolChains/Cuda.cpp:413 + + addEnvListWithSpaces(Args, CmdArgs, "CLANG_TARGET_LINK_OPTS"); + CmdArgs.push_back("-suppress-warnings"); I don't think you should invent new environment variables, Clang normally uses the `-X` class to pass arguments to specific parts of the toolchain. https://reviews.llvm.org/D42800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31696: Automatically add include-what-you-use for when building in tree
kimgr added a comment. Herald added a subscriber: kristof.beyls. Ping! It would be nice to get some traction on this. https://reviews.llvm.org/D31696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43430: Omit nullptr check for sufficiently simple delete-expressions
kimgr added a comment. Peanut gallery observation: there was a discussion on the Boost list years and years ago where someone made the case that `if (x != nullptr) delete x;` was measurably faster than just calling `delete x;` I can't find it now, but I think it might have been in the context of their `checked_delete` library. Anyway, the reasoning was that with an external nullptr check, you'd pay for one comparison, but without it you'd always pay for a jump + a comparison. I suppose that only holds true for null pointers, for non-null pointers the extra check is just waste. It looks to me like the compiler inserts an external null check, and you're now removing it in select cases, did I understand that right? I wonder if this could have negative effects for frequent deletion of nullptrs (e.g. a sometimes-allocated member of a heavily used value type). That said, I'm not sure how valid the observation back then still is. Repository: rC Clang https://reviews.llvm.org/D43430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41102: Setup clang-doc frontend framework
lebedev.ri added a comment. I don't know the protocol, but i think it might be a good idea to add a new entry to `CODE_OWNERS.TXT` for `clang-doc`? `clang-doc` going to be quite distinctive, and bigger/complicated than what already is in `clang-tools-extra`. https://reviews.llvm.org/D41102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43424: [clang-doc] Implement a (simple) Markdown generator
lebedev.ri added a comment. It will be good to have the tests for generators. Comment at: clang-doc/generators/Generators.h:28 +public: + Generator(std::unique_ptr &IS, StringRef Root, StringRef Format) : IS(IS), Root(Root), Format(Format) {}; + virtual ~Generator() {}; Is this code (and the code in two parent Differentials) formatted with `clang-format`? This line is certainly longer than 80 columns. https://reviews.llvm.org/D43424 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41102: Setup clang-doc frontend framework
lebedev.ri added a comment. Nice work! It will be great to have a replacement for doxygen, which is actually modern and usable. Some review notes for some of the code below: Comment at: clang-doc/ClangDocBinary.cpp:17 + +enum BlockIds { + NAMESPACE_BLOCK_ID = bitc::FIRST_APPLICATION_BLOCKID, I wonder if you could add a map from `BlockIds` enumerator to the textual representation. e.g. `NAMESPACE_BLOCK_ID` -> "NamespaceBlock" ... `RECORD_BLOCK_ID` -> "RecordBlock" ... This would allow to only pass the `BlockId`, and avoid passing hardcoded string each time. Comment at: clang-doc/ClangDocBinary.cpp:72 + assert(Abbrevs.find(recordID) == Abbrevs.end() && + "Abbreviation already set."); + Abbrevs[recordID] = abbrevID; So it does not *set* the abbreviation, since it is not supposed to be called if the abbreviation is already set, but it *adds* a unique abbreviation. I think it should be called `void AbbreviationMap::add(unsigned recordID, unsigned abbrevID)` then Comment at: clang-doc/ClangDocBinary.cpp:88 + Stream.Emit((unsigned)'C', 8); + Stream.Emit((unsigned)'S', 8); +} General comment: shouldn't the bitcode be versioned? Comment at: clang-doc/ClangDocBinary.cpp:99 + // Emit the block name if present. + if (!Name || Name[0] == 0) return; + Record.clear(); So you are actually checking that there is either no string, or the string is of zero length here. Is this function ever going to be called with a null `Name`? All calls in this Differential always pass a static C string here. Also see my comments about passing enumerator, and having a map that would avoid passing string altogether. Comment at: clang-doc/ClangDocBinary.cpp:120 + Abbrev->Add(BitCodeAbbrevOp(D)); + Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 16)); // String size + Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // String These constants are somewhat vague. Maybe consolidate them somewhere somehow, e.g.: ``` clang-doc/ClangDocBinary.cpp: namespace { struct BitCodeConstants { static constexpr int LineNumFixedSize = 16; ... } } ``` Comment at: clang-doc/ClangDocBinary.cpp:178 + BitstreamWriter &Stream) { + Stream.EnterSubblock(NAMED_TYPE_BLOCK_ID, 5); + emitIntRecord(ID, NAMED_TYPE_ID, Stream); From the docs i can see that `5` is `CodeLen`, but how is this decided, etc? Seems like a magical constant, maybe consolidate them somewhere, like in the previous note? Comment at: clang-doc/ClangDocBinary.h:57 + + void emitRecordID(unsigned ID, const char *Name, BitstreamWriter &Stream); + void emitBlockID(unsigned ID, const char *Name, BitstreamWriter &Stream); Should these take `StringRef` instead of `const char *` ? Comment at: clang-doc/ClangDocBinary.h:57 + + void emitRecordID(unsigned ID, const char *Name, BitstreamWriter &Stream); + void emitBlockID(unsigned ID, const char *Name, BitstreamWriter &Stream); lebedev.ri wrote: > Should these take `StringRef` instead of `const char *` ? > Also, isn't the first param always a `BlockIds`? Why not pass enumerators, and make it more obvious? Comment at: clang-doc/ClangDocBinary.h:62 + void emitIntAbbrev(unsigned D, unsigned Block, BitstreamWriter &Stream); + + RecordData Record; ^ I think all these `unsigned Block` is actually a `BlockIds Block` ? And `unsigned D` is actually `DataTypes D` ? Comment at: clang-doc/tool/ClangDocMain.cpp:41 +static cl::opt OutDirectory( +"docs", cl::desc("Directory for outputting docs."), cl::init("docs"), +cl::cat(ClangDocCategory)); Hmm, are you sure about `docs` being the param to specify where to output the docs? I'd //expect// to see `-o / --output` or a positional argument. Or is that impossible due to some parent LLVM/clang implicit requirements? Comment at: clang-doc/tool/ClangDocMain.cpp:45 +static cl::opt Format( +"format", cl::desc("Format for outputting docs. (options are md)"), +cl::init("md"), cl::cat(ClangDocCategory)); `options are: md` Though this appears to be a dead code right now Comment at: clang-doc/tool/ClangDocMain.cpp:97 + // Mapping phase + errs() << "Mapping decls...\n"; + auto Err = This does not seem to be a erroneous situation to be in Comment at: clang-doc/tool/ClangDocMain.cpp:107 + sys::path::native(OutDirectory, IRRootPath); + std::error_code DirectoryStatus = sys::fs::create_directories(IRRootPath); + if (DirectoryStatus != OK) { I'm having trouble following. `DumpResult` description says `Dump results to stdout.` Why does it need `OutDirec
[PATCH] D43105: [RISCV] Enable __int128_t and uint128_t through clang flag
asb added a comment. In https://reviews.llvm.org/D43105#1003709, @efriedma wrote: > So you want int128_t for compiler-rt itself, so you can use the soft-float > implementation, but you want to make int128_t opt-in to avoid the possibility > of someone getting a link error trying to link code built with clang against > libgcc.a? That seems a little convoluted, but I guess it's okay. That's one particular problem you might encounter if Clang and gcc for RISCV32 made different choices about enabling int128 by default. More generally, we want to ensure the C environment for both GCC and Clang is as close as possible, as any difference like this has the potential for causing difficult to debug differences in behaviour that confuse end users. Repository: rC Clang https://reviews.llvm.org/D43105 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43162: [Parser] (C++) Make -Wextra-semi slightly more useful
lebedev.ri added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D43162 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43392: [clang-tidy] Add Fuchsia checker for visibility attributes
aaron.ballman added inline comments. Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:68 + diag(MatchedDecl->getLocStart(), + "visibility attribute not set for specified function") + << MatchedDecl jakehehrlich wrote: > aaron.ballman wrote: > > How about: "function expected to be annotated with '%0' visibility" > > > > I'm mostly worried about the case where the function has a visibility > > attribute, but it has the *wrong* visibility specified. The current wording > > would be confusing in that circumstance. Or is that not a scenario you care > > about, just that *any* visibility is specified on the function? > The use case for this check is forcing code bases to carefully control what > symbols are exported rather than just exporting everything. So if someone > took the time to explicitly set the visibility of one of these symbols we > care about then we should assume they knew what they were doing. > > The specific use case I care about for this check is using > -fvisibility=hidden and then checking to make sure a certain curated list of > symbols has explicit default visibility. Ah, so a mismatch is unimportant, that's good to know. How about: "function expected to be annotated with the 'visibility' attribute"? https://reviews.llvm.org/D43392 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value
khuttun updated this revision to Diff 134802. khuttun added a comment. I changed the checker to use hasAnyName. Checking for `unique_ptr::release()` and all the `empty()` functions is removed. The checker doesn't report any warnings from LLVM + clang codebases now. https://reviews.llvm.org/D41655 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt clang-tidy/bugprone/UnusedReturnValueCheck.cpp clang-tidy/bugprone/UnusedReturnValueCheck.h docs/ReleaseNotes.rst docs/clang-tidy/checks/bugprone-unused-return-value.rst docs/clang-tidy/checks/list.rst test/clang-tidy/bugprone-unused-return-value-custom.cpp test/clang-tidy/bugprone-unused-return-value.cpp Index: test/clang-tidy/bugprone-unused-return-value.cpp === --- /dev/null +++ test/clang-tidy/bugprone-unused-return-value.cpp @@ -0,0 +1,166 @@ +// RUN: %check_clang_tidy %s bugprone-unused-return-value %t + +namespace std { + +struct future {}; + +enum class launch { + async, + deferred +}; + +template +future async(Function &&, Args &&...); + +template +future async(launch, Function &&, Args &&...); + +template +ForwardIt remove(ForwardIt, ForwardIt, const T &); + +template +ForwardIt remove_if(ForwardIt, ForwardIt, UnaryPredicate); + +template +ForwardIt unique(ForwardIt, ForwardIt); + +// the check should be able to match std lib calls even if the functions are +// declared inside inline namespaces +inline namespace v1 { + +template +T *launder(T *); + +} // namespace v1 +} // namespace std + +struct Foo { + void f(); +}; + +int increment(int i) { + return i + 1; +} + +void useFuture(const std::future &fut); + +void warning() { + std::async(increment, 42); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value] + + std::async(std::launch::async, increment, 42); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value] + + Foo F; + std::launder(&F); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value] + + std::remove(nullptr, nullptr, 1); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value] + + std::remove_if(nullptr, nullptr, nullptr); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value] + + std::unique(nullptr, nullptr); + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value] + + // test discarding return values inside different kinds of statements + + auto Lambda = [] { std::remove(nullptr, nullptr, 1); }; + // CHECK-MESSAGES: [[@LINE-1]]:22: warning: the value returned by this function should be used [bugprone-unused-return-value] + + if (true) +std::remove(nullptr, nullptr, 1); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value] + else if (true) +std::remove(nullptr, nullptr, 1); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value] + else +std::remove(nullptr, nullptr, 1); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value] + + while (true) +std::remove(nullptr, nullptr, 1); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value] + + do +std::remove(nullptr, nullptr, 1); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value] + while (true); + + for (;;) +std::remove(nullptr, nullptr, 1); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value] + + for (std::remove(nullptr, nullptr, 1);;) +// CHECK-MESSAGES: [[@LINE-1]]:8: warning: the value returned by this function should be used [bugprone-unused-return-value] +; + + for (;; std::remove(nullptr, nullptr, 1)) +// CHECK-MESSAGES: [[@LINE-1]]:11: warning: the value returned by this function should be used [bugprone-unused-return-value] +; + + for (auto C : "foo") +std::remove(nullptr, nullptr, 1); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value] + + switch (1) { + case 1: +std::remove(nullptr, nullptr, 1); +// CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value] +break; + default: +std::remove(nullptr, nullptr, 1); +// CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by
[PATCH] D43394: [X86] Add 'sahf' CPU feature to frontend
dim updated this revision to Diff 134808. dim added a comment. Remove the `__LAHFSAHF__` predefined macro. Repository: rC Clang https://reviews.llvm.org/D43394 Files: include/clang/Driver/Options.td lib/Basic/Targets/X86.cpp lib/Basic/Targets/X86.h test/CodeGen/attr-target-x86.c Index: test/CodeGen/attr-target-x86.c === --- test/CodeGen/attr-target-x86.c +++ test/CodeGen/attr-target-x86.c @@ -49,10 +49,10 @@ // CHECK: lake{{.*}} #7 // CHECK: use_before_def{{.*}} #7 // CHECK: #0 = {{.*}}"target-cpu"="i686" "target-features"="+x87" -// CHECK: #1 = {{.*}}"target-cpu"="ivybridge" "target-features"="+aes,+avx,+cx16,+f16c,+fsgsbase,+fxsr,+mmx,+pclmul,+popcnt,+rdrnd,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt" +// CHECK: #1 = {{.*}}"target-cpu"="ivybridge" "target-features"="+aes,+avx,+cx16,+f16c,+fsgsbase,+fxsr,+mmx,+pclmul,+popcnt,+rdrnd,+sahf,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt" // CHECK: #2 = {{.*}}"target-cpu"="i686" "target-features"="+x87,-aes,-avx,-avx2,-avx512bitalg,-avx512bw,-avx512cd,-avx512dq,-avx512er,-avx512f,-avx512ifma,-avx512pf,-avx512vbmi,-avx512vbmi2,-avx512vl,-avx512vnni,-avx512vpopcntdq,-f16c,-fma,-fma4,-gfni,-pclmul,-sha,-sse2,-sse3,-sse4.1,-sse4.2,-sse4a,-ssse3,-vaes,-vpclmulqdq,-xop,-xsave,-xsaveopt" // CHECK: #3 = {{.*}}"target-cpu"="i686" "target-features"="+mmx,+popcnt,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87" // CHECK: #4 = {{.*}}"target-cpu"="i686" "target-features"="+x87,-avx,-avx2,-avx512bitalg,-avx512bw,-avx512cd,-avx512dq,-avx512er,-avx512f,-avx512ifma,-avx512pf,-avx512vbmi,-avx512vbmi2,-avx512vl,-avx512vnni,-avx512vpopcntdq,-f16c,-fma,-fma4,-sse4.1,-sse4.2,-vaes,-vpclmulqdq,-xop,-xsave,-xsaveopt" -// CHECK: #5 = {{.*}}"target-cpu"="ivybridge" "target-features"="+avx,+cx16,+f16c,+fsgsbase,+fxsr,+mmx,+pclmul,+popcnt,+rdrnd,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt,-aes,-vaes" +// CHECK: #5 = {{.*}}"target-cpu"="ivybridge" "target-features"="+avx,+cx16,+f16c,+fsgsbase,+fxsr,+mmx,+pclmul,+popcnt,+rdrnd,+sahf,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt,-aes,-vaes" // CHECK: #6 = {{.*}}"target-cpu"="i686" "target-features"="+x87,-3dnow,-3dnowa,-mmx" // CHECK: #7 = {{.*}}"target-cpu"="lakemont" "target-features"="+mmx" Index: lib/Basic/Targets/X86.h === --- lib/Basic/Targets/X86.h +++ lib/Basic/Targets/X86.h @@ -99,6 +99,7 @@ bool HasRDPID = false; bool HasRetpoline = false; bool HasRetpolineExternalThunk = false; + bool HasLAHFSAHF = false; protected: /// \brief Enumeration of all of the X86 CPUs supported by Clang. Index: lib/Basic/Targets/X86.cpp === --- lib/Basic/Targets/X86.cpp +++ lib/Basic/Targets/X86.cpp @@ -220,6 +220,7 @@ LLVM_FALLTHROUGH; case CK_Core2: setFeatureEnabledImpl(Features, "ssse3", true); +setFeatureEnabledImpl(Features, "sahf", true); LLVM_FALLTHROUGH; case CK_Yonah: case CK_Prescott: @@ -261,6 +262,7 @@ setFeatureEnabledImpl(Features, "ssse3", true); setFeatureEnabledImpl(Features, "fxsr", true); setFeatureEnabledImpl(Features, "cx16", true); +setFeatureEnabledImpl(Features, "sahf", true); break; case CK_KNM: @@ -291,6 +293,7 @@ setFeatureEnabledImpl(Features, "xsaveopt", true); setFeatureEnabledImpl(Features, "xsave", true); setFeatureEnabledImpl(Features, "movbe", true); +setFeatureEnabledImpl(Features, "sahf", true); break; case CK_K6_2: @@ -304,6 +307,7 @@ setFeatureEnabledImpl(Features, "sse4a", true); setFeatureEnabledImpl(Features, "lzcnt", true); setFeatureEnabledImpl(Features, "popcnt", true); +setFeatureEnabledImpl(Features, "sahf", true); LLVM_FALLTHROUGH; case CK_K8SSE3: setFeatureEnabledImpl(Features, "sse3", true); @@ -337,6 +341,7 @@ setFeatureEnabledImpl(Features, "prfchw", true); setFeatureEnabledImpl(Features, "cx16", true); setFeatureEnabledImpl(Features, "fxsr", true); +setFeatureEnabledImpl(Features, "sahf", true); break; case CK_ZNVER1: @@ -360,6 +365,7 @@ setFeatureEnabledImpl(Features, "prfchw", true); setFeatureEnabledImpl(Features, "rdrnd", true); setFeatureEnabledImpl(Features, "rdseed", true); +setFeatureEnabledImpl(Features, "sahf", true); setFeatureEnabledImpl(Features, "sha", true); setFeatureEnabledImpl(Features, "sse4a", true); setFeatureEnabledImpl(Features, "xsave", true); @@ -394,6 +400,7 @@ setFeatureEnabledImpl(Features, "cx16", true); setFeatureEnabledImpl(Features, "fxsr", true); setFeatureEnabledImpl(Features, "xsave", true); +setFeatureEnabledImpl(Features, "sahf", true); break; } if (!TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec)) @@ -792,6 +799,8 @@ HasRetpoline = true; }
[PATCH] D43429: [clangd] Add missing library (clangLex) in a few places
ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lg. Thanks! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43429 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43394: [X86] Add 'sahf' CPU feature to frontend
craig.topper accepted this revision. craig.topper added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D43394 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43394: [X86] Add 'sahf' CPU feature to frontend
This revision was automatically updated to reflect the committed changes. Closed by commit rL325446: [X86] Add 'sahf' CPU feature to frontend (authored by dim, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D43394 Files: cfe/trunk/include/clang/Driver/Options.td cfe/trunk/lib/Basic/Targets/X86.cpp cfe/trunk/lib/Basic/Targets/X86.h cfe/trunk/test/CodeGen/attr-target-x86.c Index: cfe/trunk/include/clang/Driver/Options.td === --- cfe/trunk/include/clang/Driver/Options.td +++ cfe/trunk/include/clang/Driver/Options.td @@ -2591,6 +2591,8 @@ def mno_rtm : Flag<["-"], "mno-rtm">, Group; def mrdseed : Flag<["-"], "mrdseed">, Group; def mno_rdseed : Flag<["-"], "mno-rdseed">, Group; +def msahf : Flag<["-"], "msahf">, Group; +def mno_sahf : Flag<["-"], "mno-sahf">, Group; def msgx : Flag<["-"], "msgx">, Group; def mno_sgx : Flag<["-"], "mno-sgx">, Group; def msha : Flag<["-"], "msha">, Group; Index: cfe/trunk/test/CodeGen/attr-target-x86.c === --- cfe/trunk/test/CodeGen/attr-target-x86.c +++ cfe/trunk/test/CodeGen/attr-target-x86.c @@ -49,10 +49,10 @@ // CHECK: lake{{.*}} #7 // CHECK: use_before_def{{.*}} #7 // CHECK: #0 = {{.*}}"target-cpu"="i686" "target-features"="+x87" -// CHECK: #1 = {{.*}}"target-cpu"="ivybridge" "target-features"="+aes,+avx,+cx16,+f16c,+fsgsbase,+fxsr,+mmx,+pclmul,+popcnt,+rdrnd,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt" +// CHECK: #1 = {{.*}}"target-cpu"="ivybridge" "target-features"="+aes,+avx,+cx16,+f16c,+fsgsbase,+fxsr,+mmx,+pclmul,+popcnt,+rdrnd,+sahf,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt" // CHECK: #2 = {{.*}}"target-cpu"="i686" "target-features"="+x87,-aes,-avx,-avx2,-avx512bitalg,-avx512bw,-avx512cd,-avx512dq,-avx512er,-avx512f,-avx512ifma,-avx512pf,-avx512vbmi,-avx512vbmi2,-avx512vl,-avx512vnni,-avx512vpopcntdq,-f16c,-fma,-fma4,-gfni,-pclmul,-sha,-sse2,-sse3,-sse4.1,-sse4.2,-sse4a,-ssse3,-vaes,-vpclmulqdq,-xop,-xsave,-xsaveopt" // CHECK: #3 = {{.*}}"target-cpu"="i686" "target-features"="+mmx,+popcnt,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87" // CHECK: #4 = {{.*}}"target-cpu"="i686" "target-features"="+x87,-avx,-avx2,-avx512bitalg,-avx512bw,-avx512cd,-avx512dq,-avx512er,-avx512f,-avx512ifma,-avx512pf,-avx512vbmi,-avx512vbmi2,-avx512vl,-avx512vnni,-avx512vpopcntdq,-f16c,-fma,-fma4,-sse4.1,-sse4.2,-vaes,-vpclmulqdq,-xop,-xsave,-xsaveopt" -// CHECK: #5 = {{.*}}"target-cpu"="ivybridge" "target-features"="+avx,+cx16,+f16c,+fsgsbase,+fxsr,+mmx,+pclmul,+popcnt,+rdrnd,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt,-aes,-vaes" +// CHECK: #5 = {{.*}}"target-cpu"="ivybridge" "target-features"="+avx,+cx16,+f16c,+fsgsbase,+fxsr,+mmx,+pclmul,+popcnt,+rdrnd,+sahf,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt,-aes,-vaes" // CHECK: #6 = {{.*}}"target-cpu"="i686" "target-features"="+x87,-3dnow,-3dnowa,-mmx" // CHECK: #7 = {{.*}}"target-cpu"="lakemont" "target-features"="+mmx" Index: cfe/trunk/lib/Basic/Targets/X86.cpp === --- cfe/trunk/lib/Basic/Targets/X86.cpp +++ cfe/trunk/lib/Basic/Targets/X86.cpp @@ -220,6 +220,7 @@ LLVM_FALLTHROUGH; case CK_Core2: setFeatureEnabledImpl(Features, "ssse3", true); +setFeatureEnabledImpl(Features, "sahf", true); LLVM_FALLTHROUGH; case CK_Yonah: case CK_Prescott: @@ -261,6 +262,7 @@ setFeatureEnabledImpl(Features, "ssse3", true); setFeatureEnabledImpl(Features, "fxsr", true); setFeatureEnabledImpl(Features, "cx16", true); +setFeatureEnabledImpl(Features, "sahf", true); break; case CK_KNM: @@ -291,6 +293,7 @@ setFeatureEnabledImpl(Features, "xsaveopt", true); setFeatureEnabledImpl(Features, "xsave", true); setFeatureEnabledImpl(Features, "movbe", true); +setFeatureEnabledImpl(Features, "sahf", true); break; case CK_K6_2: @@ -304,6 +307,7 @@ setFeatureEnabledImpl(Features, "sse4a", true); setFeatureEnabledImpl(Features, "lzcnt", true); setFeatureEnabledImpl(Features, "popcnt", true); +setFeatureEnabledImpl(Features, "sahf", true); LLVM_FALLTHROUGH; case CK_K8SSE3: setFeatureEnabledImpl(Features, "sse3", true); @@ -337,6 +341,7 @@ setFeatureEnabledImpl(Features, "prfchw", true); setFeatureEnabledImpl(Features, "cx16", true); setFeatureEnabledImpl(Features, "fxsr", true); +setFeatureEnabledImpl(Features, "sahf", true); break; case CK_ZNVER1: @@ -360,6 +365,7 @@ setFeatureEnabledImpl(Features, "prfchw", true); setFeatureEnabledImpl(Features, "rdrnd", true); setFeatureEnabledImpl(Features, "rdseed", true); +setFeatureEnabledImpl(Features, "sahf", true); setFeatureEnabledImpl(Features, "sha", true); setFeatureEnabledImpl(Features, "sse4a", true); setFe
r325446 - [X86] Add 'sahf' CPU feature to frontend
Author: dim Date: Sat Feb 17 13:04:35 2018 New Revision: 325446 URL: http://llvm.org/viewvc/llvm-project?rev=325446&view=rev Log: [X86] Add 'sahf' CPU feature to frontend Summary: Make clang accept `-msahf` (and `-mno-sahf`) flags to activate the `+sahf` feature for the backend, for bug 36028 (Incorrect use of pushf/popf enables/disables interrupts on amd64 kernels). This was originally submitted in bug 36037 by Jonathan Looney . As described there, GCC also uses `-msahf` for this feature, and the backend already recognizes the `+sahf` feature. All that is needed is to teach clang to pass this on to the backend. The mapping of feature support onto CPUs may not be complete; rather, it was chosen to match LLVM's idea of which CPUs support this feature (see lib/Target/X86/X86.td). I also updated the affected test case (CodeGen/attr-target-x86.c) to match the emitted output. Reviewers: craig.topper, coby, efriedma, rsmith Reviewed By: craig.topper Subscribers: emaste, cfe-commits Differential Revision: https://reviews.llvm.org/D43394 Modified: cfe/trunk/include/clang/Driver/Options.td cfe/trunk/lib/Basic/Targets/X86.cpp cfe/trunk/lib/Basic/Targets/X86.h cfe/trunk/test/CodeGen/attr-target-x86.c Modified: cfe/trunk/include/clang/Driver/Options.td URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=325446&r1=325445&r2=325446&view=diff == --- cfe/trunk/include/clang/Driver/Options.td (original) +++ cfe/trunk/include/clang/Driver/Options.td Sat Feb 17 13:04:35 2018 @@ -2591,6 +2591,8 @@ def mrtm : Flag<["-"], "mrtm">, Group, Group; def mrdseed : Flag<["-"], "mrdseed">, Group; def mno_rdseed : Flag<["-"], "mno-rdseed">, Group; +def msahf : Flag<["-"], "msahf">, Group; +def mno_sahf : Flag<["-"], "mno-sahf">, Group; def msgx : Flag<["-"], "msgx">, Group; def mno_sgx : Flag<["-"], "mno-sgx">, Group; def msha : Flag<["-"], "msha">, Group; Modified: cfe/trunk/lib/Basic/Targets/X86.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/X86.cpp?rev=325446&r1=325445&r2=325446&view=diff == --- cfe/trunk/lib/Basic/Targets/X86.cpp (original) +++ cfe/trunk/lib/Basic/Targets/X86.cpp Sat Feb 17 13:04:35 2018 @@ -220,6 +220,7 @@ bool X86TargetInfo::initFeatureMap( LLVM_FALLTHROUGH; case CK_Core2: setFeatureEnabledImpl(Features, "ssse3", true); +setFeatureEnabledImpl(Features, "sahf", true); LLVM_FALLTHROUGH; case CK_Yonah: case CK_Prescott: @@ -261,6 +262,7 @@ bool X86TargetInfo::initFeatureMap( setFeatureEnabledImpl(Features, "ssse3", true); setFeatureEnabledImpl(Features, "fxsr", true); setFeatureEnabledImpl(Features, "cx16", true); +setFeatureEnabledImpl(Features, "sahf", true); break; case CK_KNM: @@ -291,6 +293,7 @@ bool X86TargetInfo::initFeatureMap( setFeatureEnabledImpl(Features, "xsaveopt", true); setFeatureEnabledImpl(Features, "xsave", true); setFeatureEnabledImpl(Features, "movbe", true); +setFeatureEnabledImpl(Features, "sahf", true); break; case CK_K6_2: @@ -304,6 +307,7 @@ bool X86TargetInfo::initFeatureMap( setFeatureEnabledImpl(Features, "sse4a", true); setFeatureEnabledImpl(Features, "lzcnt", true); setFeatureEnabledImpl(Features, "popcnt", true); +setFeatureEnabledImpl(Features, "sahf", true); LLVM_FALLTHROUGH; case CK_K8SSE3: setFeatureEnabledImpl(Features, "sse3", true); @@ -337,6 +341,7 @@ bool X86TargetInfo::initFeatureMap( setFeatureEnabledImpl(Features, "prfchw", true); setFeatureEnabledImpl(Features, "cx16", true); setFeatureEnabledImpl(Features, "fxsr", true); +setFeatureEnabledImpl(Features, "sahf", true); break; case CK_ZNVER1: @@ -360,6 +365,7 @@ bool X86TargetInfo::initFeatureMap( setFeatureEnabledImpl(Features, "prfchw", true); setFeatureEnabledImpl(Features, "rdrnd", true); setFeatureEnabledImpl(Features, "rdseed", true); +setFeatureEnabledImpl(Features, "sahf", true); setFeatureEnabledImpl(Features, "sha", true); setFeatureEnabledImpl(Features, "sse4a", true); setFeatureEnabledImpl(Features, "xsave", true); @@ -394,6 +400,7 @@ bool X86TargetInfo::initFeatureMap( setFeatureEnabledImpl(Features, "cx16", true); setFeatureEnabledImpl(Features, "fxsr", true); setFeatureEnabledImpl(Features, "xsave", true); +setFeatureEnabledImpl(Features, "sahf", true); break; } if (!TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec)) @@ -792,6 +799,8 @@ bool X86TargetInfo::handleTargetFeatures HasRetpoline = true; } else if (Feature == "+retpoline-external-thunk") { HasRetpolineExternalThunk = true; +} else if (Feature == "+sahf") { + HasLAHFSAHF = true; } X86SSEEnum Level = llvm::StringSwitch(Feature) @@ -1269,6 +1278,7 @@
[PATCH] D43404: [Fuchsia] Include libClang and clang-include-fixer in the toolchain
jakehehrlich accepted this revision. jakehehrlich added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D43404 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41629: [libcxx] Improve accuracy of complex asinh and acosh
mclow.lists accepted this revision. mclow.lists added a comment. This revision is now accepted and ready to land. LGTM. Do you need me to commit this? https://reviews.llvm.org/D41629 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits