[PATCH] D50678: [InlineAsm] Update the min-legal-vector-width function attribute based on inputs and outputs to inline assembly

chandlerc added a comment. This makes sense to me, but definitely wait for someone more familiar w/ Clang's IR gen to review... https://reviews.llvm.org/D50678 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/ma

[PATCH] D47814: Teach libc++ to use native NetBSD's max_align_t

chandlerc added a comment. In https://reviews.llvm.org/D47814#1206372, @krytarowski wrote: > If there are no more comments, I will land this by the end of this week. Just for the record, this is not OK and not how LLVM's code review works. You can and must wait for review. I think Joerg alread

[PATCH] D47814: Teach libc++ to use native NetBSD's max_align_t

chandlerc added a comment. (To be clear, this continues to not be related to this patch, but happy to discuss...) Comment at: include/stddef.h:55 // Re-use the compiler's max_align_t where possible. #if !defined(__CLANG_MAX_ALIGN_T_DEFINED) && !defined(_GCC_MAX_ALIGN_T) &

[PATCH] D51150: [x86/retpoline] Split the LLVM concept of retpolines into separate subtarget features for indirect calls and indirect branches.

chandlerc created this revision. chandlerc added reviewers: echristo, rnk, craig.topper. Herald added subscribers: hiraditya, mcrosier, sanjoy. Herald added a reviewer: javed.absar. This is in preparation for enabling *only* the call retpolines when using speculative load hardening. I've continue

[PATCH] D51150: [x86/retpoline] Split the LLVM concept of retpolines into separate subtarget features for indirect calls and indirect branches.

chandlerc added a comment. Thanks! I'm going to go ahead and land this, but happy to iterate on anything if others have comments. Repository: rL LLVM https://reviews.llvm.org/D51150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:/

[PATCH] D51150: [x86/retpoline] Split the LLVM concept of retpolines into separate subtarget features for indirect calls and indirect branches.

This revision was automatically updated to reflect the committed changes. Closed by commit rC340515: [x86/retpoline] Split the LLVM concept of retpolines into separate (authored by chandlerc, committed by ). Changed prior to commit: https://reviews.llvm.org/D51150?vs=162130&id=162138#toc Repos

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

chandlerc created this revision. chandlerc added reviewers: rsmith, rnk, echristo, craig.topper, kristof.beyls. Herald added subscribers: jfb, dexonsmith, steven_wu, hiraditya, eraman, mcrosier, mehdi_amini, sanjoy. Wires up the existing pass to work with a proper IR attribute rather than just a

[PATCH] D46593: Allow copy elision in path concatenation

chandlerc added a comment. Sorry folks, but you can't just take patches to libstdc++ and apply them to libc++. These libraries have different licenses, and so the author of the patch (Jonathan Wakely in this case) need's to *explicitly* contribute that patch to libc++ under libc++'s license. (

[PATCH] D46593: Allow copy elision in path concatenation

chandlerc added a comment. In https://reviews.llvm.org/D46593#1093758, @jwakely wrote: > @chandlerc thanks for catching this. > > As the original author I agree to contribute this patch to libc++ under the > terms of the MIT and the University of Illinois licences, and under the terms > of "Apa

[PATCH] D38824: [X86] Synchronize the CPU predefined macros with gcc

chandlerc added inline comments. Comment at: lib/Basic/Targets/X86.cpp:844-845 -// FIXME: Historically, we defined this legacy name, it would be nice to -// remove it at some point. We've never exposed fine-grained names for -// recent primary x86 CPUs, and we should

[PATCH] D38824: [X86] Synchronize the CPU predefined macros with gcc

chandlerc added inline comments. Comment at: lib/Basic/Targets/X86.cpp:844-845 -// FIXME: Historically, we defined this legacy name, it would be nice to -// remove it at some point. We've never exposed fine-grained names for -// recent primary x86 CPUs, and we should

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

chandlerc updated this revision to Diff 162319. chandlerc marked 3 inline comments as done. chandlerc added a comment. Address review comments. Repository: rL LLVM https://reviews.llvm.org/D51157 Files: clang/include/clang/Driver/Options.td clang/include/clang/Frontend/CodeGenOptions.def

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

chandlerc added a comment. Thanks, should all be addressed now. Comment at: clang/lib/Driver/ToolChains/Arch/X86.cpp:169-170 options::OPT_mno_retpoline_external_thunk, false)) { // FIXME: Add a warning about failing to specify `-mretpoline` and

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

chandlerc updated this revision to Diff 162322. chandlerc added a comment. Add a test file that I somehow missed earlier (sorry about that). Repository: rL LLVM https://reviews.llvm.org/D51157 Files: clang/include/clang/Driver/Options.td clang/include/clang/Frontend/CodeGenOptions.def

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

chandlerc updated this revision to Diff 162325. chandlerc added a comment. Move to a more conservative model suggested by Kristof. Repository: rL LLVM https://reviews.llvm.org/D51157 Files: clang/include/clang/Driver/Options.td clang/include/clang/Frontend/CodeGenOptions.def clang/lib/

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

chandlerc marked an inline comment as done. chandlerc added inline comments. Comment at: llvm/docs/LangRef.rst:1659-1661 +that hardening. It should also be possible to *not* harden a hot and/or safe +function and have code inlined there *not* be hardened (even if the gen

[PATCH] D51567: CMake: Consolidate gtest detection code

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. I mean, sure. I really don't know that supporting this ever expanding diversity of build strategies is worth its cost, but I don't see a specific reason to not take this patch Rep

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

chandlerc marked 5 inline comments as done. chandlerc added a comment. All outstanding comments addressed, and landing this. Thanks all for the reviews and let me know if I missed anything. Comment at: llvm/include/llvm/IR/Attributes.td:181-185 +/// Note that this uses the def

[PATCH] D51157: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.

This revision was automatically updated to reflect the committed changes. chandlerc marked an inline comment as done. Closed by commit rL341363: [x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative (authored by chandlerc, committed by ). Changed prior to commit: https://reviews

[PATCH] D50055: Update the coding standard about NFC changes and whitespace

chandlerc added inline comments. Comment at: docs/DeveloperPolicy.rst:395-408 +Commits with No Functional Change +- + +It may be permissible to commit changes without prior review when the changes +have no semantic impact on the code if the changes

[PATCH] D50055: Update the coding standard about NFC changes and whitespace

chandlerc added inline comments. Comment at: docs/DeveloperPolicy.rst:395-408 +Commits with No Functional Change +- + +It may be permissible to commit changes without prior review when the changes +have no semantic impact on the code if the changes

[PATCH] D50055: Update the coding standard about NFC changes and whitespace

chandlerc accepted this revision. chandlerc added a comment. This looks really good to me and seems like a nice clarification of historical practice. =D Thanks so much for driving an actual documentation update for folks that simply would never know about these practices otherwise, I think it w

[PATCH] D50055: Update the coding standard about NFC changes and whitespace

chandlerc added inline comments. Comment at: docs/CodingStandards.rst:514-516 +line of code. Some common editors will automatically remove trailing whitespace +when saving a file which causes unrelated changes to appear in diffs and +commits. Meinersbur wrote: >

[PATCH] D43203: [Driver] Generate .eh_frame_hdr for static executables too.

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. Agreed. Repository: rC Clang https://reviews.llvm.org/D43203 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.o

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

chandlerc added a comment. (focusing on the LLVM side of this review for now) Can you add an LLVM-based test? Can you add this to `lib/Passes/PassRegistry.def`? Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:423 + +class AARGetter { + FunctionAnalysisManager &A

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

chandlerc added a comment. In https://reviews.llvm.org/D33525#766050, @timshen wrote: > In https://reviews.llvm.org/D33525#764251, @chandlerc wrote: > > > (focusing on the LLVM side of this review for now) > > > > Can you add an LLVM-based test? Can you add this to > > `lib/Passes/PassRegistry.d

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

chandlerc added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:913-914 +std::error_code EC; +ThinLinkOS.emplace(CodeGenOpts.ThinLinkBitcodeFile, EC, + llvm::sys::fs::F_None); +if (EC) { The clang s

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

chandlerc added inline comments. Comment at: llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll:1 +; RUN: opt -passes='lto' -debug-pass-manager -thinlto-bc -thin-link-bitcode-file=%t2 -o %t %s 2>&1 | FileCheck %s --check-prefix=DEBUG_PM +; RUN: llvm-bcanalyzer -dump %t2 | FileC

[PATCH] D33692: [ThinLTO] Wire up ThinLTO and new PM

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. This patch LGTM whenever the underlying LLVM change lands, thanks https://reviews.llvm.org/D33692 ___ cfe-commits mailing list cfe-comm

[PATCH] D33932: [clang-format] Add support for case-insensitive header matching and use it to improve support for LLVM-style include sorting.

chandlerc created this revision. Herald added subscribers: mcrosier, sanjoy, klimek. This really is a collection of improvements to the rules for LLVM include sorting: - We have gmock headers now, so it adds support for those to one of the categories. - LLVM does use 'FooTest.cpp' files to test

[PATCH] D33932: [clang-format] Add support for case-insensitive header matching and use it to improve support for LLVM-style include sorting.

chandlerc updated this revision to Diff 101544. chandlerc added a comment. Add test coverage for the case-insensitive category logic. https://reviews.llvm.org/D33932 Files: include/clang/Format/Format.h lib/Format/Format.cpp unittests/Format/SortIncludesTest.cpp Index: unittests/Format/S

[PATCH] D33932: [clang-format] Add support for case-insensitive header matching and use it to improve support for LLVM-style include sorting.

chandlerc marked an inline comment as done. chandlerc added a comment. Added more tests, PTAL? https://reviews.llvm.org/D33932 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33932: [clang-format] Add support for case-insensitive header matching and use it to improve support for LLVM-style include sorting.

chandlerc updated this revision to Diff 103885. chandlerc marked an inline comment as done. chandlerc added a comment. Update based on review comments. https://reviews.llvm.org/D33932 Files: include/clang/Format/Format.h lib/Format/Format.cpp unittests/Format/SortIncludesTest.cpp Index:

[PATCH] D33932: [clang-format] Add support for case-insensitive header matching and use it to improve support for LLVM-style include sorting.

chandlerc requested review of this revision. chandlerc added a comment. Tried to address the test comment -- let me know if something else was intended. https://reviews.llvm.org/D33932 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://l

[PATCH] D36836: [clang-tidy] Implement sonarsource-function-cognitive-complexity check

chandlerc added a comment. In https://reviews.llvm.org/D36836#931995, @lebedev.ri wrote: > - Rebased > - As advised by @aaron.ballman, moved into it's own directory/module. Please > review that, i'm not entirely sure i have done that fully correctly. > > @chandlerc Hi! @aaron.ballman has sugge

[PATCH] D43995: Do not generate calls to fentry with __attribute__((no_instrument_function))

chandlerc edited reviewers, added: rnk, rsmith, rjmccall; removed: chandlerc. chandlerc added a comment. Adding more Clang CodeGen folks to this review rather than myself... Repository: rC Clang https://reviews.llvm.org/D43995 ___ cfe-commits mai

[PATCH] D42787: clang-format: do not add extra indent when wrapping last parameter

chandlerc added a comment. In https://reviews.llvm.org/D42787#1000687, @krasimir wrote: > We could adapt the single-argument version instead, turning: > > foo(bb + > c); > > > into: > > foo(bb + > c); > I have a

[PATCH] D6260: Add -mlong-double-64 flag

chandlerc edited subscribers, added: brooksmoses, chandlerc; removed: rnk. chandlerc edited reviewers, added: echristo, timshen, dlj; removed: kcc. chandlerc added a comment. Ok folks, I know this is a blast from the past, but can we actually review this? I'm willing to commandeer it, rebase it,

[PATCH] D48464: [x86] Teach the builtin argument range check to allow invalid ranges in dead code.

chandlerc created this revision. chandlerc added reviewers: craig.topper, rsmith. Herald added subscribers: llvm-commits, atanasyan, kbarton, nemanjai, mcrosier, sanjoy. Herald added a reviewer: javed.absar. This is important for C++ templates that essentially compute the valid input in a way tha

[PATCH] D48462: [X86] Update handling in CGBuiltin to be tolerant of out of range immediates.

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

[PATCH] D48464: [x86] Teach the builtin argument range check to allow invalid ranges in dead code.

This revision was automatically updated to reflect the committed changes. Closed by commit rC335309: [x86] Teach the builtin argument range check to allow invalid ranges in (authored by chandlerc, committed by ). Changed prior to commit: https://reviews.llvm.org/D48464?vs=152404&id=152411#toc

[PATCH] D56690: [Nios2] Remove Nios2 backend

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. LGTM as a patch, but same as the other wait to land until the -dev thread settles. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56690/new/ https://reviews.llvm.org/D56690 _

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

chandlerc added a subscriber: hans. chandlerc added a comment. Adding Hans so he can be aware of the progress. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56571/new/ https://reviews.llvm.org/D56571 ___ cfe-commits mailing list cfe-commits

[PATCH] D56353: Replace cc1 options '-mdisable-fp-elim' and '-momit-leaf-frame-pointer' with'-mframe-pointer='

chandlerc requested changes to this revision. chandlerc added a comment. This revision now requires changes to proceed. Wow, thanks for the cleanups. This is much easier to read as a consequence. And sorry it took a while to get you another round of review. Comments below. Com

[PATCH] D56932: [Driver] [NetBSD] Pass default library search paths to linker

chandlerc added a comment. There was a long discussion between two NetBSD maintainers about this (both already in the reviewers list of this patch). I'm not sure if there is an existing thread that would be better to follow up on as opposed to starting a fresh thread. I think the question was:

[PATCH] D54547: PTH-- Remove feature entirely-

chandlerc added a comment. Should likely add release notes about this. Also might be worth sending a note to cfe-dev as a heads up and give folks some time to say "wait wait". Repository: rC Clang https://reviews.llvm.org/D54547 ___ cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

chandlerc requested changes to this revision. chandlerc added a comment. This revision now requires changes to proceed. I think this should be `internal-driver-option` and the text updated? I don't think these are necessarily experimental, they're internals of the compiler implementation, and no

[PATCH] D58374: [Clang][NewPM] Don't bail out if the target machine is empty

chandlerc requested changes to this revision. chandlerc added a comment. This revision now requires changes to proceed. Maybe update at least some of the tests using these targets to additionally run with the new pass manager explicitly enabled via flag? Comment at: clang/lib/

[PATCH] D61022: [ThinLTO] Pass down opt level to LTO backend and handle -O0 LTO in new PM

chandlerc added inline comments. Comment at: llvm/test/tools/gold/X86/opt-level.ll:53 + ; CHECK-O1-OLDPM: select + ; The new PM does not do as many optimizations at O1 + ; CHECK-O1-NEWPM: phi tejohnson wrote: > tejohnson wrote: > > mehdi_amini wrote: > > > Thi

[PATCH] D61138: [PGO] Enable InstrProf lowering for Clang PGO instrumentation in the new pass manager

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. Awesome, LGTM You might also update one of the instr prof tests to have two `RUN` lines, one for each pass manager. Feel free to do that (or not) and submit. CHANGES SINCE LAST ACTION

[PATCH] D61142: Set LoopInterleaved in the PassManagerBuilder.

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. LGTM, maybe add a comment here to archive some of the reasoning? (IE, that when unrolling is disabled we disable both literal unrolling but also interleaving for historical reasons) Re

[PATCH] D58374: [Clang][NewPM] Don't bail out if the target machine is empty

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. LGTM! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58374/new/ https://reviews.llvm.org/D58374 ___ cfe-com

[PATCH] D61620: [NewPassManager] Add tuning option: LoopUnrolling [clang-change]

chandlerc added a comment. Same comment as on other Clang patch -- let's update an IR generation test to reflect this? Should just be adding a RUN line. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61620/new/ https://reviews.llvm.org/D61620

[PATCH] D61617: [NewPassManager] Add tuning option: SLPVectorization [clang-change]

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. Can you update some Clang IR generation test that uses these flags to run w/ the new PM? It should fail without this and pass with this. LGTM w/ that test updated. Repository: rC Cla

[PATCH] D61617: [NewPassManager] Add tuning option: SLPVectorization [clang-change]

chandlerc requested changes to this revision. chandlerc added a comment. This revision now requires changes to proceed. The file name of the test seems odd? How about `vectorize-loops.c`? I'd also make it a C test and put it in `test/CodeGen` instead of a C++ test. Comment at:

[PATCH] D61617: [NewPassManager] Add tuning option: SLPVectorization [clang-change]

chandlerc added a comment. (Sorry for still more comments) Comment at: test/CodeGen/loop-vectorize.c:10 +// CHECK-DISABLE-VECT-LABEL: @for_test() +// CHECK-DISABLE-VECT: fmul double + You'll want to check that the vector form *doesn't* show up. I would expect

[PATCH] D57640: [NewPM][MSan] Add Options Handling

chandlerc added a comment. Minor past-commit nit. Comment at: lib/CodeGen/BackendUtil.cpp:1039 [](FunctionPassManager &FPM, PassBuilder::OptimizationLevel Level) { - FPM.addPass(MemorySanitizerPass()); + FPM.addPass(MemorySanitizerPass({}

[PATCH] D28248: Work around GCC PR37804

chandlerc added a comment. Hey Marshall and Michael, As mentioned in my email to all the lists[1], patches posted to Phabricator before the new license was installed should be confirmed as under the new license before being rebased and applied. Not sure that happened here as the file headers a

[PATCH] D58375: [Clang][NewPM] Disable tests that are broken under new PM

chandlerc requested changes to this revision. chandlerc added a comment. This revision now requires changes to proceed. Based on the -dev discussion, update once the target machine differences are addressed by mimicing the way the legacy PM works, which will hopefully restrict this similarly to

[PATCH] D58375: [Clang][NewPM] Disable tests that are broken under new PM

chandlerc added a comment. In D58375#1403144 , @efriedma wrote: > I can understand why tests that use -O1 or -O2 would produce different > results with the new pass manager, but it looks like not all the tests are > like that. Do you know why those test

[PATCH] D58424: [NewPM] Add other sanitizers at O0

chandlerc accepted this revision. chandlerc 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/D58424/new/ https://reviews.llvm.org/D58424 _

[PATCH] D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule.

chandlerc added a comment. But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here... https://reviews.llv

[PATCH] D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule.

chandlerc added a comment. In https://reviews.llvm.org/D38042#875328, @aprantl wrote: > In https://reviews.llvm.org/D38042#875303, @chandlerc wrote: > > > But, the verifier itself will just "crash". It won't print a stack trace, > > but I don't see why that's much better? And this flag is suppos

[PATCH] D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule.

chandlerc added a comment. In https://reviews.llvm.org/D38042#875412, @aprantl wrote: > In https://reviews.llvm.org/D38042#875334, @chandlerc wrote: > > > In https://reviews.llvm.org/D38042#875328, @aprantl wrote: > > > > > In https://reviews.llvm.org/D38042#875303, @chandlerc wrote: > > > > > >

[PATCH] D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule.

chandlerc added a comment. In https://reviews.llvm.org/D38042#875441, @aprantl wrote: > In https://reviews.llvm.org/D38042#875418, @chandlerc wrote: > > > Absolutely. I think the verifier should never, under any circumstances, > > mutate the IR. Think about it, with the current design if a pass

[PATCH] D38042: EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule.

chandlerc requested changes to this revision. chandlerc added a comment. This revision now requires changes to proceed. Marking as needing changes to clear dashboard. https://reviews.llvm.org/D38042 ___ cfe-commits mailing list cfe-commits@lists.llv

[PATCH] D62888: [NewPM] Port Sancov

chandlerc requested changes to this revision. chandlerc added a comment. This revision now requires changes to proceed. I would just change this to have the module pass loop over the functions -- that seems like it'll be much cleaner. As it is, I'm not seeing where the loop actually happens. But

[PATCH] D62225: [clang][NewPM] Fixing -O0 tests that are broken under new PM

chandlerc requested changes to this revision. chandlerc added a comment. This revision now requires changes to proceed. I think this ultimately needs to be split up into smaller patches. A bunch of these things can be landed independently. Here is my first cut at things to split out, each one in

[PATCH] D62888: [NewPM] Port Sancov

chandlerc requested changes to this revision. chandlerc added a comment. This revision now requires changes to proceed. This was a lot easier for me to understand too, thanks. Somewhat minor code changes below. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1231-1232 + MPM

[PATCH] D63174: [clang][NewPM] Add RUNS for tests that produce slightly different IR under new PM

chandlerc added a comment. This really confused me. We shouldn't be seeing this kind of difference in the new PM. But I think I figured it out. Both PMs have to run *some* inliner at -O0. This is because we need to inline `always_inline` functions. The new PM has a *super* simple (and fast co

[PATCH] D63153: [clang][NewPM] Fix broken -O0 test from the AlwaysInliner

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. Code change LGTM. Can you update at least one of the tests to explicitly run both PMs so that we'll notice if this breaks in some weird way? Feel free to submit with that change. Repos

[PATCH] D63155: [clang][NewPM] Fix broken profile test

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. Let's update the test to explicitly run w/ both PMs to make sure this keeps working. LGTM with that change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D63156: [clang][NewPM] Add -fno-experimental-new-pass-manager to tests

chandlerc added a comment. I understand the change to explicitly say `-O2`. I also understand the change to add an explicit `-fno-experimental-new-pass-manager` to a `RUN` line when we have another `RUN` line that explicitly uses `-fexperiemntal-new-pass-manager`. But for many of these cases, t

[PATCH] D63168: [clang][NewPM] Fix split debug test

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. Code change LGTM, but again, let's update the test to check both ways. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63168/new/ https://re

[PATCH] D63170: [clang][NewPM] Fix broken -O0 test from missing assumptions

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. LGTM. Bit annoying that we need to do this at O0, but fine... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63170/new/ https://reviews.llv

[PATCH] D63156: [clang][NewPM] Add -fno-experimental-new-pass-manager to tests

chandlerc added a comment. In D63156#1543937 , @leonardchan wrote: > I did some more digging and found the following differences between PMs for > each test, and they seem to all differ and can be fixed for different reasons. Awesome, and sorry this is

[PATCH] D63174: [clang][NewPM] Add RUNS for tests that produce slightly different IR under new PM

chandlerc added a comment. OMG, I'm so sorry, I had no idea that the tests would explode like that... Yeah, I don't think that's useful Maybe a better approach is to just explicitly run the code through `opt -passes=instsimplify` before handing it to FileCheck? That should produce almost i

[PATCH] D62225: [clang][NewPM] Fixing remaining -O0 tests that are broken under new PM

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. FWIW, this LGTM. We may want to tweak the behavior around flattening, but that can happen as a follow-up, and this seems to get us to a better state. Repository: rG LLVM Github Monore

[PATCH] D62888: [NewPM] Port Sancov

chandlerc added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1231-1232 + MPM.addPass(ModuleSanitizerCoveragePass(SancovOpts)); + MPM.addPass( + createModuleToFunctionPassAdaptor(SanitizerCoveragePass(SancovOpts))); +} leo

[PATCH] D63577: [clang][NewPM] Move EntryExitInstrumenterPass to the start of the pipeline

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. LGTM, nice! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63577/new/ https://reviews.llvm.org/D63577 __

[PATCH] D63580: [clang][NewPM] Do not eliminate available_externally durng `-O2 -flto` runs

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. LGTM, again, really nice. Tiny tweak below. Comment at: llvm/test/Other/available-externally-lto.ll:1 +; Ensure that we don't emit available_externally functions at -O2

[PATCH] D63156: [clang][NewPM] Add -fno-experimental-new-pass-manager to tests

chandlerc added a comment. just a minor comment on one of these... Comment at: clang/test/CodeGen/pgo-sample.c:10 + +// The new pass manager analog to PrunEH is a combination of 'function-attrs' +// and 'function(simplify-cgf)'. s/PrunEH/PruneEH/

[PATCH] D62888: [NewPM] Port Sancov

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. LGTM, thanks so much for sticking through this, I know it was ... nontrivial! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62888/new/ htt

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

chandlerc requested changes to this revision. chandlerc added a comment. This revision now requires changes to proceed. Sorry for the delay here. It'd be nice to land the LLVM patch first and with its own testing -- we should have testing for the pass builder independent of Clang (IE, in the LLV

[PATCH] D63638: [clang][NewPM] Add new pass manager RUN lines to avx512f-builtins.c

chandlerc added a comment. Just to make sure we're on the same page (and sorry I didn't jump in sooner)... With the old PM, *anything* that is `always_inline` *gets* `instsimplify` run on it, even at -O0, even if you didn't want that. So using `-instsimplify` explicitly is, IMO, not any more sc

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. LGTM with two nits addressed, thanks! Comment at: clang/lib/CodeGen/BackendUtil.cpp:1125 +PB.addPGOInstrPassesForO0(MPM, CodeGenOpts.DebugPassManager, +

[PATCH] D65410: [PassManager] First Pass implementation at -O1 pass pipeline

chandlerc added a comment. One high level point that is at least worth clarifying, and maybe others will want to suggest a different approach: The overall approach here is to have as small of a difference between the O1 and O2

[PATCH] D63156: [clang][NewPM] Add -fno-experimental-new-pass-manager to tests

chandlerc accepted this revision. chandlerc 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/D63156/new/ https://reviews.llvm.org/D63156 _

[PATCH] D63626: [clang][NewPM] Remove exception handling before loading pgo sample profile data

chandlerc added reviewers: davidxl, tejohnson. chandlerc added a comment. See inline comment, but I think we should just drop the testing of the function attribute bit here rather than adjusting the pipeline. Comment at: llvm/lib/Passes/PassBuilder.cpp:665-668 +// We must

[PATCH] D63174: [clang][NewPM] Add RUNS for tests that produce slightly different IR under new PM

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. Eh, this seems close enough now. I'd like a better approach for the x86 builtins, but no idea what it will end up being. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D63626: [clang][NewPM] Remove exception handling before loading pgo sample profile data

chandlerc added a comment. FWIW, I think we can wait for a bug to be filed or report come in with some functional test case before we change any behavior here. I'm just suggesting how to make the test better below. Comment at: llvm/lib/Passes/PassBuilder.cpp:665-668 +// W

[PATCH] D63626: [clang][NewPM] Remove exception handling before loading pgo sample profile data

chandlerc accepted this revision. chandlerc 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/D63626/new/ https://reviews.llvm.org/D63626 _

[PATCH] D63155: [clang][NewPM] Fix broken profile test

chandlerc added a comment. In D63155#1563229 , @xur wrote: > This patch does not make sense to me. > > The reason of failing with -fexperimental-new-pass-manager is because we > don't do PGO instrumentation at -O0. This is due to the fact that PGO > inst

[PATCH] D63155: [clang][NewPM] Fix broken profile test

chandlerc added a comment. In D63155#1563240 , @leonardchan wrote: > In D63155#1563229 , @xur wrote: > > > This patch does not make sense to me. > > > > The reason of failing with -fexperimental-new-pass-manager is

[PATCH] D63155: [clang][NewPM] Fix broken profile test

chandlerc added a comment. In D63155#1563275 , @xur wrote: > In D63155#1563266 , @chandlerc wrote: > > > I just think we also need to understand why *no other test failed*, and why > > the only test we have for doi

[PATCH] D62888: [NewPM] Port Sancov

chandlerc added a comment. The used thing still seems like there is an underlynig bug here. See below. (Also a tiny nit, but that isn't the interesting part.) Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:206 + +std::string getSectionStart(const Triple

[PATCH] D61617: [NewPassManager] Add tuning option: SLPVectorization [clang-change]

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61617/new/ https://reviews.llvm.org/D61617 ___ cfe-comm

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

chandlerc added a comment. Sorry I've been a bit slow to respond here... In D61634#1503089 , @hfinkel wrote: > In D61634#1502201 , @tejohnson wrote: > > > In D61634#1502138

[PATCH] D55915: [Driver] Make -fno-omit-frame-pointer imply -mno-omit-leaf-frame-pointer

chandlerc requested changes to this revision. chandlerc added inline comments. This revision now requires changes to proceed. Comment at: lib/CodeGen/CGCall.cpp:1739 FuncAttrs.addAttribute("no-frame-pointer-elim", "true"); - FuncAttrs.addAttribute("no-frame-pointer-el

[PATCH] D55915: [Driver] Make -fno-omit-frame-pointer imply -mno-omit-leaf-frame-pointer

chandlerc requested changes to this revision. chandlerc added reviewers: hans, rnk. chandlerc added a comment. This revision now requires changes to proceed. The number of negations and such in the CC1 interface here and the attributes makes all of these tests super confusing. Wonder if we should

  1   2   3   >