[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

[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 A->getOption().matches(options::OPT_fno_omit_fr

[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/ https://reviews.l

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

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

chandlerc added a comment. These kinds of improvements to warnings are awesome, but the way we are deploying them presents serious challenges to adoption which I think we need to address. I think significant new warning functionality should as a matter of course go behind some warning group at

[PATCH] D45505: [WIP] [GCC] Match a GCC version with a patch suffix without a third version component

chandlerc requested changes to this revision. chandlerc added a comment. This revision now requires changes to proceed. Definitely need tests here. But no concern w/ the idea of this. Repository: rC Clang https://reviews.llvm.org/D45505 ___ cfe-

[PATCH] D45505: [WIP] [GCC] Match a GCC version with a patch suffix without a third version component

chandlerc added a comment. In https://reviews.llvm.org/D45505#1074747, @mstorsjo wrote: > In https://reviews.llvm.org/D45505#1074744, @chandlerc wrote: > > > Definitely need tests here. > > > Do you have any suggestion on where to create them and what kind? Just a > normal test with a fake direc

[PATCH] D45505: [GCC] Match a GCC version with a patch suffix without a third version component

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

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

chandlerc added inline comments. Comment at: docs/UsersManual.rst:1260-1265 + Enable or disable a code generation optimization that may convert a + cast of a floating-point value to integer and back to floating-point + into the equivalent of the math libary's 'trunc()' func

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

chandlerc added inline comments. Comment at: docs/UsersManual.rst:1260-1265 + Enable or disable a code generation optimization that may convert a + cast of a floating-point value to integer and back to floating-point + into the equivalent of the math libary's 'trunc()' func

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

chandlerc added a comment. In https://reviews.llvm.org/D46135#1080108, @spatel wrote: > 3. Remove the 'no' version of the flag. Given the change in the default, this > seems more natural to me, and it simplifies the patch/tests...but I might > have been too pessimistic before and this is too op

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

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

[PATCH] D48617: [Builtins][Attributes][X86] Tag all X86 builtins with their required vector width. Add a min_vector_width function attribute and tag all x86 instrinsics with it.

chandlerc added a comment. FWIW, I looked at an early version of this patch and am generally happy with the target-specific / IR-specific behavior aspects of it. Totally leaving the detailed review of the attribute stuff to you Aaron, as you're already doing an amazing job there. Minor clarific

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

chandlerc resigned from this revision. chandlerc added a comment. I don't have any problem with this patch's code in theory but: 1. The question of whether this macro approach is better or worse than FreeBSD's or MUSL's seems a question for libc++ maintainers, not me. 2. The question of whether

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

[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: clang/w

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

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

[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 Clang's getting involved document and make it more inclusive in wording. (authored by chandlerc). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

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

[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] 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 = +&CGAM.getResult(*C, CG); +ResultFAMCP->updateFAM(FAM);

[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 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 = +&CGAM

[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: clang/test/Mis

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

[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 https://review

[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

chandlerc added a comment. I'm really not a fan of the degree of complexity and subtlety that this introduces into the frontend, all to allow particular backend optimizations. I feel like this is Clang working around a fundamental deficiency in LLVM and we should instead find a way to fix this

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

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

[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 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: clang/lib/Driver/ToolChains/CommonArgs

[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 https://reviews.llvm.

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

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

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

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

[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", +], GMNG

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

[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: https://re

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

<    1   2   3   4   5   6   >