[PATCH] D157188: [clang-tidy] Add bugprone-new-bool-conversion

chandlerc added a comment. In D157188#4563143 , @Eugene.Zelenko wrote: > In D157188#4563139 , @PiotrZSL > wrote: > >> In D157188#4563105 , >> @Eugene.Zelenko wrote: >>

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

This revision was automatically updated to reflect the committed changes. Closed by commit rG0198d76e1e76: [Bazel] Get `//clang` building on Windows with clang-cl. (authored by chandlerc). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

chandlerc added inline comments. Comment at: utils/bazel/llvm-project-overlay/clang/BUILD.bazel:1828 ], -copts = [ -"-Wno-uninitialized", -], +copts = select({ +"@bazel_tools//src/conditions:windows": [], rnk wrote: > Enabling

[PATCH] D112883: [bazel] Re-introduce `copts` hacks for lib/AST includes.

chandlerc added a comment. In D112883#3101685 , @GMNGeoffrey wrote: >> Is this breaking actual users? I wouldn't expect it to break unless they >> depend on this target. > > I don't know :-) I just know that people have sent me patches to remove the >

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

chandlerc added a comment. Thanks, making suggested changes and then landing! Comment at: utils/bazel/llvm-project-overlay/clang/BUILD.bazel:364 +# Clang-specific define on non-Windows platforms. +"CLANG_HAVE_RLIMITS=1", +],

[PATCH] D112883: [bazel] Re-introduce `copts` hacks for lib/AST includes.

chandlerc added a comment. In D112883#3101645 , @GMNGeoffrey wrote: > :-( Did you try the separate library with `strip_include_prefix`? I can add a separate library to do that. It would still expose all consumers to `Opcodes.inc` instead of only

[PATCH] D112883: [bazel] Re-introduce `copts` hacks for lib/AST includes.

This revision was automatically updated to reflect the committed changes. Closed by commit rGd1fdd745d510: Re-introduce `copts` hacks for lib/AST includes. (authored by chandlerc). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Repository: rG LLVM Github Monorepo

[PATCH] D112883: [bazel] Re-introduce `copts` hacks for lib/AST includes.

chandlerc created this revision. chandlerc added reviewers: GMNGeoffrey, dblaikie. Herald added a subscriber: mcrosier. chandlerc requested review of this revision. Sadly, these are necessary AFAICT. There is a file `lib/AST/CXXABI.h`. On case insensitive file systems like macOS this will collide

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

chandlerc requested review of this revision. chandlerc added a comment. PTAL, and thanks for feedback so far! Comment at: clang/include/clang/Basic/Builtins.def:1059 +#undef strcasecmp +#undef strncasecmp + rnk wrote: > thakis wrote: > > GMNGeoffrey wrote: > >

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

chandlerc updated this revision to Diff 383634. chandlerc marked 5 inline comments as done. chandlerc edited the summary of this revision. chandlerc added a reviewer: rnk. chandlerc added a comment. Major update to better fix some of the issues here. No longer requires any changes outside of

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

chandlerc added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:1059 +#undef strcasecmp +#undef strncasecmp + thakis wrote: > Why do we need this with bazel but not with other windows builds? I don't know how this never was hit by other

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

chandlerc created this revision. chandlerc added a reviewer: GMNGeoffrey. Herald added a subscriber: mcrosier. chandlerc requested review of this revision. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. This required substantially more invasive changes I'm afraid.

[PATCH] D80488: Teach `-fsanitize=fuzzer` to respect `-static` and `-static-libstdc++` when adding C++ standard libraries.

This revision was automatically updated to reflect the committed changes. Closed by commit rGaaf7ffd4e1aa: Teach `-fsanitize=fuzzer` to respect `-static` and `-static-libstdc++` when… (authored by chandlerc). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D80488: Teach `-fsanitize=fuzzer` to respect `-static` and `-static-libstdc++` when adding C++ standard libraries.

chandlerc updated this revision to Diff 300842. chandlerc marked an inline comment as done. chandlerc added a comment. Update with a regression test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80488/new/ https://reviews.llvm.org/D80488 Files:

[PATCH] D80488: Teach `-fsanitize=fuzzer` to respect `-static` and `-static-libstdc++` when adding C++ standard libraries.

chandlerc added a comment. In D80488#2341009 , @vitalybuka wrote: > LGTM, but it could be better with a test Thanks, added test and landing. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80488/new/ https://reviews.llvm.org/D80488

[PATCH] D80488: Teach `-fsanitize=fuzzer` to respect `-static` and `-static-libstdc++` when adding C++ standard libraries.

chandlerc added a reviewer: vitalybuka. chandlerc marked an inline comment as done. chandlerc added a comment. Ping (and adding some sanitizer folks)? I'd really love to stop building with this local patch Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:754 +

[PATCH] D88789: [InstCombine] Revert rL226781 "Teach InstCombine to canonicalize loads which are only ever stored to always use a legal integer type if one is available." (PR47592)

chandlerc added a comment. In D88789#2310967 , @efriedma wrote: > In D88789#2310606 , @chandlerc wrote: > >> FWIW, I still very much feel that this is the correct canonicalization, and >> that downstream problems

[PATCH] D87837: [Driver] Add disabled-by-default -Wuse-ld-path for the deprecation warning for -fuse-ld=/abs/path

chandlerc added a comment. Good to confirm with Dave of course, but this looks pretty great to me, and thanks so much! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87837/new/ https://reviews.llvm.org/D87837

[PATCH] D87837: [Driver] Remove the deprecation warning for -fuse-ld=/abs/path

chandlerc added a comment. FWIW, I would like to see *something* like this (both in the release and on trunk) but not sure this is actually the patch we want... Clang typically uses `FIXME` comments, and doesn't leave commented out code... I feel like this should be an off-by-default warning,

[PATCH] D71687: Fix full loop unrolling initialization in new pass manager

chandlerc accepted this revision. chandlerc added a comment. Cool, thanks and LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71687/new/ https://reviews.llvm.org/D71687 ___ cfe-commits mailing

[PATCH] D80488: Teach `-fsanitize=fuzzer` to respect `-static` and `-static-libstdc++` when adding C++ standard libraries.

chandlerc created this revision. Herald added subscribers: cfe-commits, mcrosier. Herald added a project: clang. No idea if this is 'correct' or the right way to fix this, so just sending this mostly as an FYI. Someone who works more closely on the sanitizers might need to take it over and figure

[PATCH] D71687: Fix full loop unrolling initialization in new pass manager

chandlerc added inline comments. Comment at: clang/test/Misc/loop-opt-setup.c:12 +// Check br i1 to make sure that the loop is fully unrolled // CHECK-NOT: br i1 This is dead now that you have different prefixes... Comment at:

[PATCH] D72893: [NewPassManager] Add assertions when getting statefull cached analysis.

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. LGTM other than two nits here, this is really awesome! Comment at: llvm/include/llvm/Analysis/CGSCCPassManager.h:856-858 +auto *ResultFAMCP = +(*C,

[PATCH] D71687: Fix full loop unrolling initialization in new pass manager

chandlerc added a comment. Wooot about finally having a test case! (Sorry for nit picking it a bit ) Comment at: llvm/test/Transforms/LoopUnroll/FullUnroll.ll:4-6 +; We don't end up deleting the loop, but we remove everything inside of it so checking for any +; reasonable

[PATCH] D72893: [NewPassManager] Add assertions when getting statefull cached analysis.

chandlerc added a comment. Really like the approach now. Pretty minor code nits below only. =D Comment at: llvm/include/llvm/Analysis/CGSCCPassManager.h:856-858 +auto *ResultFAMCP = +(*C, CG); +ResultFAMCP->updateFAM(FAM); Check

[PATCH] D75936: Add a Pass to X86 that builds a Condensed CFG for Load Value Injection (LVI) Gadgets [4/6]

chandlerc added inline comments. Comment at: llvm/lib/Target/X86/ImmutableGraph.h:46-56 + using NodeValueT = _NodeValueT; + using EdgeValueT = _EdgeValueT; + using size_type = _SizeT; + class Node; + class Edge { +friend class ImmutableGraph; +template friend class

[PATCH] D71687: Fix full loop unrolling initialization in new pass manager

chandlerc added a comment. Can we add an LLVM test w/ the metadata so that we have an entirely LLVM test flow that ensures the pass builder DTRT? (I still would include the Clang side test which is also very useful to test integrating Clang w/ different flows through the pass manager.)

[PATCH] D70157: Align branches within 32-Byte boundary

chandlerc added a comment. Just wanted to say thanks for the performance data! I know it was hard to get, but it is really, really useful to help folks evaluate these kinds of changes with actual data around the options available. CHANGES SINCE LAST ACTION

[PATCH] D70157: Align branches within 32-Byte boundary

chandlerc added a comment. I'm seeing lots of updates to fix bugs, but no movement for many days on both my meta comments and (in some ways more importantly) James's meta comments. (And thanks Philip for chiming in too!) Meanwhile, we really, really need to get this functionality in place. The

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

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. Minor nits around redundant predicates for SROA. With thouse fixed, LGTM. I'd really love to find a way to make TCO debuggable so that we don't lose that. I'm particularly worried about

[PATCH] D70157: Align branches within 32-Byte boundary

chandlerc added a comment. (Just a reminder that we need to have both performance and code size numbers for this patch. And given that there are a few options, may need a few examples.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70157/new/ https://reviews.llvm.org/D70157

[PATCH] D70157: Align branches within 32-Byte boundary

chandlerc added a reviewer: chandlerc. chandlerc added a comment. Thanks for sending this out! I've made a minor comment on the flag structure, but my bigger question would be: can you summarize the performance hit and code size hit you've seen across some benchmarks? SPEC and the LLVM test

[PATCH] D69351: Improve Clang's getting involved document and make it more inclusive in wording.

This revision was automatically updated to reflect the committed changes. Closed by commit rGdc1499b90dc4: Improve Clangs getting involved document and make it more inclusive in wording. (authored by chandlerc). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D69351: Improve Clang's getting involved document and make it more inclusive in wording.

chandlerc updated this revision to Diff 226171. chandlerc marked an inline comment as done. chandlerc added a comment. Address feedback from Manuel. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69351/new/ https://reviews.llvm.org/D69351 Files:

[PATCH] D69351: Improve Clang's getting involved document and make it more inclusive in wording.

chandlerc marked 2 inline comments as done. chandlerc added a comment. Thanks, updated. Comment at: clang/www/get_involved.html:113 + + A high-quality implementation: The implementation must fit well into + Clang's architecture, follow LLVM's coding conventions, and meet

[PATCH] D69351: Improve Clang's getting involved document and make it more inclusive in wording.

chandlerc updated this revision to Diff 226174. chandlerc marked 3 inline comments as done. chandlerc added a comment. More fixes from Manuel. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69351/new/ https://reviews.llvm.org/D69351 Files:

[PATCH] D69351: Improve Clang's getting involved document and make it more inclusive in wording.

chandlerc created this revision. chandlerc added a reviewer: klimek. Herald added a subscriber: mcrosier. Herald added a project: clang. Working with Meike and others to improve the wording in this document. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D69351 Files:

[PATCH] D68535: Fix loop unrolling initialization in the new pass manager

chandlerc accepted this revision. chandlerc added a reviewer: asbirlea. chandlerc added a comment. This revision is now accepted and ready to land. Adding Alina so she is aware of the change and can comment if she spots anything I'm missing... I think this is fine to go in as-is to fix the

[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

chandlerc added inline comments. Comment at: compiler-rt/trunk/lib/profile/xxhash.h:41-42 + +#include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/StringRef.h" + Sorry folks, but you can't do this. You can't depend on ADT from compiler-rt currently. There are at

[PATCH] D67323: [NewPM][Sancov] Create the Sancov Pass after building the pipelines

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. LGTM to fix folks broken by this. That said, we should look for a way to test this in a follow up patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D66988: [NewPM][Sancov] Make Sancov a Module Pass instead of 2 Passes

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. LGTM This also makes sense to me why it would OOM. I suspect we're building a massive list, and at best we get lucky and they get de-duped later. At worst, this is actually fixing a

[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] 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] 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

[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

[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/

[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] 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

[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#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 >

[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] 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 +//

[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

[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] 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] 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] 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

[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] 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))); +}

[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

[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

[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] 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/

[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/

[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,

[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

[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

[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.

[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 +

[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

[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.

[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] 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 ___

[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] 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

[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

[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] 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 ___

[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)

[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] 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: > > >

[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:

[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] 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

[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] 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

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

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

[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

[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.

[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

[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] D56112: [clang-offload-bundler] Added install component

chandlerc edited reviewers, added: ABataev, hfinkel; removed: chandlerc. chandlerc added a comment. Adding likely more useful reviewers... Not really interesting from a CMake perspective. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56112/new/

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

chandlerc added inline comments. Comment at: lib/Driver/ToolChains/Clang.cpp:592-595 + if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer, + options::OPT_fomit_frame_pointer)) +return

[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

[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"); -

[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

[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 ___

[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:

[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

[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

  1   2   3   >