[PATCH] D43002: Emit S_OBJNAME symbol in CodeView

echristo added a comment. First question: Since split dwarf has to do some similar things can we not use the same support? This seems to be a lot of changes for this. Second question: and if we can't use the same support can we make split dwarf use this? Having two separate methods for passin

[PATCH] D79117: [clang] [Darwin] Add reverse mappings for aarch64/aarch64_32 to darwin arch names

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. This seems ok to me. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79117/new/ https://reviews.llvm.org/D79117 ___

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

echristo updated this revision to Diff 267424. echristo marked an inline comment as done. echristo added a comment. Update and restructure some test cases. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71687/new/ https://reviews.llvm.org/D71687 Fi

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

echristo marked 7 inline comments as done. echristo added a comment. Done. New diff incoming. 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 chandlerc wrote: > This is dead now

[PATCH] D80883: [Driver] Add multiclass OptInFlag and OptOutFlag to simplify boolean option definition

echristo accepted this revision. echristo added a comment. This looks good to me. Might want to open a project around migrating everything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80883/new/ https://reviews.llvm.org/D80883

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

echristo added a comment. This seems like something that we should then revert until we know that instsimplify can be updated with a fix? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83360/new/ https://reviews.llvm.org/D83360 _

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

echristo added a comment. We're starting to see miscompiles as we do more testing as well, just nothing smaller at the moment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83360/new/ https://reviews.llvm.org/D83360

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

echristo added a comment. It's that even before the msan instrumentation the IR doesn't look correct - thus a miscompile. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83360/new/ https://reviews.llvm.org/D83360 _

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

echristo added a comment. In D83360#2154637 , @aqjune wrote: > In D83360#2154584 , @echristo wrote: > > > It's that even before the msan instrumentation the IR doesn't look correct > > - thus a miscompile. > > > I'

[PATCH] D84058: Pass -rtlib=libgcc in tests conditioned on the default.

echristo added a reviewer: saugustine. echristo added a comment. (Adding Sterling as well) I think this will be an OK workaround for now, go ahead and reply on the main thread as I think they'll want to set the default runtime library as part of the toolchain. I'm surprised at this behavior as w

[PATCH] D63852: [Clang] Move assembler into a separate file

echristo added a comment. Hmm. In general I'd like to hear more about what you're trying. Can you give an idea of how you'd like to invoke the library to assemble something via clang? Why not just call into llvm directly? I'm not necessarily against changing the layering here fwiw, just wanted

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

echristo added reviewers: chandlerc, asbirlea. echristo added a comment. Adding Chandler and Alina here as well. In general, I don't think that this is such a great idea. Being able to have this sort of thing work more reliably is one of the reasons for the new pass manager. I think I'd like to

[PATCH] D83273: [X86] Remove the feature dependency handling in X86TargetInfo::setFeatureEnabledImpl to a table based lookup in X86TargetParser.cpp

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Works for me :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83273/new/ https://reviews.llvm.org/D83273 ___ cfe-commits mailing lis

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

echristo added a comment. Some inline nits. I see you've already committed and that's fine - I still don't think we should do it, but we can delete it again soon :) Comment at: clang/lib/CodeGen/BackendUtil.cpp:623 PMBuilder.LoopVectorize = CodeGenOpts.VectorizeLoop; + PMB

[PATCH] D83897: [X86] Add a generic 32-bit CPU with sse2 with modern tuning flags to be used as the default for the 32-bit targets instead of pentium4

echristo added a comment. Did you want to just change the pentium4 tuning? Otherwise naming things is hard and this feels awkward, but I don't have any better ideas :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83897/new/ https://reviews.llvm.org/D83897 __

[PATCH] D82244: [SanitizeCoverage] Rename -fsanitize-coverage-{white,black}list to -fsanitize-coverage-{allow,block}list

echristo added a reviewer: HAPPY. echristo added a comment. You'll want to grab blacklist/whitelist in the comments around as well. We've been talking about having a need for deprecated aliases file for these options - it's probably worth bringing this patch up on cfe-dev and cc'ing richardsmit

[PATCH] D82244: [SanitizeCoverage] Rename -fsanitize-coverage-{white,black}list to -fsanitize-coverage-{allow,block}list

echristo added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:313-316 +.. option:: -mharden-sls= + +Select straight-line speculation hardening scope + Miss merge? Comment at: clang/docs/ClangCommandLineReference.rst:891

[PATCH] D82244: [SanitizeCoverage] Rename -fsanitize-coverage-{white,black}list to -fsanitize-coverage-{allow,block}list

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Couple of inline comments then looks good to go. Thank you so much for your effort here. -eric Comment at: clang/docs/ClangCommandLineReference.rst:891 Restrict sani

[PATCH] D82244: [SanitizeCoverage] Rename -fsanitize-coverage-{white,black}list to -fsanitize-coverage-{allow,block}list

echristo added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:891 Restrict sanitizer coverage instrumentation exclusively to modules and functions that match the provided special case list, except the blacklisted ones MaskRay wrote: >

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

echristo closed this revision. echristo added a comment. Committed a while back. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71687/new/ https://reviews.llvm.org/D71687 ___ cfe-commits mailing list cf

[PATCH] D77954: [CUDA][HIP] Fix host/device based overload resolution

echristo added a subscriber: erichkeane. echristo added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:9749 + if (isBetterMultiversionCandidate(Cand1, Cand2)) +return true; + rjmccall wrote: > yaxunl wrote: > > rjmccall wrote: > > > If we move

[PATCH] D79210: Let clang print registered targets for --version

echristo added a comment. LGTM if it works? Can you post the output into the commit message so we can review that here? I guess clang -v or something? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79210/new/ https://reviews.llvm.org/D79210 ___

[PATCH] D79210: Let clang print registered targets for --version

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Yeah, it's fine. We might want to put it behind a "long information" option, but for now it's ok. Keep an eye open if someone complains please. CHANGES SINCE LAST ACTION https://reviews

[PATCH] D83273: [X86] Remove the feature dependency handling in X86TargetInfo::setFeatureEnabledImpl to a table based lookup in X86TargetParser.cpp

echristo added a comment. In D83273#2194842 , @nickdesaulniers wrote: > In D83273#2194832 , @craig.topper > wrote: > >> It's a pretty nasty revert in our downstream tree where we have prototyped >> future ISAs. S

[PATCH] D84068: AMDGPU/clang: Search resource directory for device libraries

echristo added a comment. Hi All, I'd really like to avoid depending on any program other than clang installing something into clang's working directory. I think this needs to be written from the perspective of specifying another directory to look for things. -eric CHANGES SINCE LAST ACTION

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

echristo updated this revision to Diff 261928. echristo edited the summary of this revision. echristo added a comment. Herald added a subscriber: zzheng. Add a testcase with opt and command line option so we can enable it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

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

echristo updated this revision to Diff 262458. echristo added a comment. Update and reduce testcase a bit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71687/new/ https://reviews.llvm.org/D71687 Files: clang/test/Misc/loop-opt-setup.c llvm/li

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

echristo marked 6 inline comments as done. echristo added a comment. OK, ready again :) 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 instruction from

[PATCH] D79916: Map -O to -O1 instead of -O2

echristo accepted this revision. echristo added a reviewer: dexonsmith. echristo added a comment. This revision is now accepted and ready to land. I'm totally down, but you knew that already :) Duncan: Do you have any concerns? I doubt it, but just checking. Repository: rG LLVM Github Monorep

[PATCH] D127812: [AArch64] Function multiversioning support added.

echristo added a comment. In D127812#3616603 , @danielkiss wrote: > In D127812#3602688 , @aaron.ballman > wrote: > >> In D127812#3602645 , @erichkeane >> wrote: >> >>>

[PATCH] D36249: Mark tests that need intel 80-bit floats as x86-only

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. I'm ok with this. Repository: rL LLVM https://reviews.llvm.org/D36249 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D35449: [X86] Implement __builtin_cpu_is

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. SGTM. -eric https://reviews.llvm.org/D35449 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/li

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

echristo added a comment. Sadly I'm just noticing this: I don't really agree with the rationale for adding fortran support to the clang driver. My proposed design here would be to move the aspects you need out of clang and into a separate library in llvm that you can use instead. I don't neces

[PATCH] D89184: Support complex target features combinations

echristo added a comment. Hi Peng, Looks interesting, but I think the language support needs some more comments about what's expected and in addition through everything else please? I've added some inline comments with places I think could use it for sure. -eric Comment at:

[PATCH] D89184: Support complex target features combinations

echristo requested changes to this revision. echristo added a comment. This revision now requires changes to proceed. Mark as requesting changes :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89184/new/ https://reviews.llvm.org/D89184 ___ c

[PATCH] D63852: [Clang] Move assembler into a separate file

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Thanks for the explanation, lgtm. -eric Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63852/new/ https://reviews.llvm.org/D63852 _

[PATCH] D89184: Support complex target features combinations

echristo added a comment. I'll take a look tomorrow, sorry for the delay. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89184/new/ https://reviews.llvm.org/D89184 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/

[PATCH] D89184: Support complex target features combinations

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Let's go ahead and unblock you, but getting a lot of this refactored would be great if you can. I think it's hitting the limits of the original design. :) CHANGES SINCE LAST ACTION http

[PATCH] D89158: [NewPM] Run callbacks added via registerPipelineStartEPCallback under -O0

echristo added a comment. In D89158#2363220 , @aeubanks wrote: > Looking at BackendUtil.cpp in Clang as well as the Rust code, I'm back to > thinking that we should provide a way to to all callbacks. Then in the case > of passes added via TargetMachine,

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

echristo added a comment. I'm also not a fan of this change. From a project perspective the binary is clang and while people may wish to change the name for their own product teams it seems like that onus should be on them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

[PATCH] D96203: [clang][patch] Modify sanitizer options names: renaming blacklist to blocklist

echristo added a reviewer: vitalybuka. echristo added a comment. In D96203#2548495 , @aaron.ballman wrote: > In D96203#2548471 , @mibintc wrote: > >> In D96203#2546856 , @aa

[PATCH] D96906: [AMDGPU] gfx90a support

echristo added a comment. In D96906#2572842 , @msearles wrote: > In D96906#2572749 , @kzhuravl wrote: > >>> The point is that nobody upstream even got a chance to chime in. >> >> We are and will be taking care of an

[PATCH] D96203: [clang][patch] Modify sanitizer options names: renaming blacklist to blocklist

echristo added a comment. Exclusion list? or exclude? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96203/new/ https://reviews.llvm.org/D96203 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http

[PATCH] D51650: Implement target_clones multiversioning

echristo added a comment. FWIW I'm a bit rusty in this area myself, but thanks for doing this. Let's see if we can't get Aaron to continue reviewing :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51650/new/ https://reviews.llvm.org/D51650

[PATCH] D97736: [Driver] Add a experimental option to link to LLVM libc.

echristo added a comment. In addition to the bikeshed below, I'm not a huge fan of this in general. I think we should assume that the libc we link is complete and thus it would just be named "libc." and in a sysroot somewhere. Another option is perhaps looking at the rtlib option, but I'd want

[PATCH] D110549: [HIPSPV][1/4] Refactor HIP tool chain

echristo added inline comments. Comment at: clang/lib/Driver/ToolChains/HIPUtility.cpp:119-133 + // Add MC directives to embed target binaries. We ensure that each + // section and image is 16-byte aligned. This is not mandatory, but + // increases the likelihood of data to be

[PATCH] D141547: Fix format for `case` in .proto files

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

echristo accepted this revision. echristo added a comment. Explicitly still ok with this as well. Thanks for continuing here. :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86310/new/ https://reviews.llvm.org/D86310 _

[PATCH] D75152: Fix typo

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. This probably qualifies as "obvious" :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75152/new/ https://reviews.llvm.org/D75152

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

echristo updated this revision to Diff 228338. echristo marked an inline comment as done. echristo added a comment. Herald added subscribers: aheejin, sbc100, nhaehnle, jvesely. I've gone ahead and enabled SROA here. In the testing I've done so far it's helped execute time quite a bit and compile

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

echristo updated this revision to Diff 228339. echristo added a comment. Update to remove comments around SROA addition. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65410/new/ https://reviews.llvm.org/D65410 Files: clang/test/CodeGen/2008-07-30-implicit-initialization.c clang/tes

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

echristo added a comment. Ping :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65410/new/ https://reviews.llvm.org/D65410 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

echristo added a comment. In D62731#1749916 , @mibintc wrote: > Thanks I see it, I'm working on a patch. Previously there was no support for > frounding-math (unimplemented). This patch enables the option. In the IR > builder, there's a call to a runtime

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG8ff85ed905a7: As a follow-up to my initial mail to llvm-dev here's a first pass at the O1… (authored by echristo). Changed prior to commit: https://reviews.llvm.org/D65410?vs=228339&id=230987#toc Repos

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

This revision was not accepted when it landed; it landed in state "Needs Revision". This revision was automatically updated to reflect the committed changes. Closed by commit rGfd39b1bb20ce: Revert "Revert "As a follow-up to my initial mail to llvm-dev here's a first… (authored by echristo). Hera

[PATCH] D71320: [IR] Split out target specific intrinsic enums into separate headers

echristo accepted this revision. echristo added a comment. SGTM and thank you :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71320/new/ https://reviews.llvm.org/D71320 ___ cfe-commits mailing list cf

[PATCH] D71393: Default to -fno-use-init-array

echristo added a comment. Fan of this change, but let's definitely wait for more reviews :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71393/new/ https://reviews.llvm.org/D71393 ___ cfe-commits mail

[PATCH] D71486: [clang-tools-extra] Fix switch coverage warning

echristo created this revision. echristo added reviewers: Anastasia, sammccall. echristo added a project: clang-tools-extra. Herald added subscribers: cfe-commits, mcrosier. Herald added a project: clang. echristo added a subscriber: rdhindsa. ... I think this is right? Add support in the switch

[PATCH] D71486: [clang-tools-extra] Fix switch coverage warning

echristo updated this revision to Diff 233857. echristo added a comment. Fix for some slightly better API uses. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71486/new/ https://reviews.llvm.org/D71486 Files: clang-tools-extra/clang-tidy/ClangTid

[PATCH] D71486: [clang-tools-extra] Fix switch coverage warning

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG5623bd52acd3: Fix -Wswitch-coverage warning in clang-tidy after ak_addrspace introduction. (authored by echristo). Reposi

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

echristo updated this revision to Diff 234656. echristo added a comment. Formatting and parens changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71687/new/ https://reviews.llvm.org/D71687 Files: clang/test/Misc/loop-opt-setup.c llvm/lib/P

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

echristo created this revision. echristo added reviewers: chandlerc, hfinkel, asbirlea. Herald added subscribers: llvm-commits, cfe-commits, hiraditya, mcrosier. Herald added projects: clang, LLVM. echristo updated this revision to Diff 234656. echristo added a comment. Formatting and parens chang

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

echristo added a comment. Ping ping goes the trolley. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71687/new/ https://reviews.llvm.org/D71687 ___ cfe-commits mailing list cfe-commits@lists.llvm.org ht

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

echristo updated this revision to Diff 250229. echristo added a comment. Fixed the clang test. Tried to get something that I could reduce down and duplicate with just opt but it's been... difficult. Even the small clang testcase in isolation won't duplicate via something like: clang -O0 -fexpe

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

echristo updated this revision to Diff 250232. echristo added a comment. Fix comments around full unroller. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71687/new/ https://reviews.llvm.org/D71687 Files: clang/test/Misc/loop-opt-setup.c llvm/l

[PATCH] D76297: [clang][AArch64] readd support for 'p' inline asm constraint

echristo added inline comments. Comment at: clang/test/CodeGen/inline-asm-p-constraint.c:1 +// RUN: %clang_cc1 -emit-llvm %s -o - -triple aarch64-linux-gnu | FileCheck %s +void foo(void* ptr) { nickdesaulniers wrote: > Does this need `REQUIRES: aarch64-registered

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

echristo added a comment. In D70172#1879481 , @jpienaar wrote: > This seems to result in triggering clang/lib/CodeGen/CGExpr.cpp:2626 when > compiling mlir/lib/Transforms/AffineDataCopyGeneration.cpp with clang build > with assertions on (clean build at

[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles

echristo added inline comments. Comment at: clang/include/clang/Frontend/CodeGenOptions.h:113 +XRay_None,// Emit none of the instrumentation points. +XRay_FunctionExtents, // Only emit function entry/exit instrumentation + // points. -

[PATCH] D45061: [NVPTX, CUDA] Use custom feature detection to handle NVPTX target builtins.

echristo added a comment. The llvm change and corresponding switch from satom->sm_60 in the front end is fine. Let's talk about the rest of it more. I'm not sure I'm seeing the need here rather than the annotations that are already here. Can you elaborate more here on why we need an additional

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

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Uh sure. :) Though I'd almost do it the other way around. That said, -mllvm probably has more uptake. Repository: rC Clang https://reviews.llvm.org/D45109 _

[PATCH] D45061: [NVPTX, CUDA] Improved feature constraints on NVPTX target builtins.

echristo accepted this revision. echristo added a comment. Guessing that SM_60 (etc) are defines? Anyhow LGTM. I'm not sure you can split up the satom part of the patch, but if you can that'd be great. -eric https://reviews.llvm.org/D45061 ___ cf

[PATCH] D45474: [XRay][clang+compiler-rt] Support build-time mode selection

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Sure. Comment at: clang/lib/Driver/XRayArgs.cpp:168 - for (const auto&AttrFile : AttrListFiles) { + for (const auto &AttrFile : AttrListFiles) { SmallString<64>

[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles

echristo added inline comments. Comment at: clang/include/clang/Frontend/CodeGenOptions.h:110 + enum XRayInstrumentationPointBundle { +XRay_All, // Always emit all the instrumentation points. dberris wrote: > pelikan wrote: > > To me, this feel

[PATCH] D39341: [X86][Driver] Move all of the X86 feature flags to one spot in the Options.td file and pair them up with their negations.

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

[PATCH] D40228: [Target] Keep the TargetOptions feature list sorted instead of sorting during CodeGen

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. This seems strictly more difficult to keep under control? Though I guess the assert helps. Feel free to go ahead, but... https://reviews.llvm.org/D40228 __

[PATCH] D40228: [Target] Keep the TargetOptions feature list sorted instead of sorting during CodeGen

echristo added a comment. No strong opinion. I think I like that one more though. https://reviews.llvm.org/D40228 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28037: [PowerPC, DAGCombiner] Change vec_sl to a << (b % (sizeof(a) * 8)), and fold it back to a << b.

echristo added a comment. One inline comment for the clang part and then let's split the llvm part out into a separate patch please. Thanks! -eric Comment at: clang/lib/Headers/altivec.h:8045 /* vec_sl */ static __inline__ vector unsigned char __ATTRS_o_ai --

[PATCH] D28037: [Altivec] Change vec_sl to a << (b % (sizeof(a) * 8))

echristo added a comment. LGTM. Thanks! -eric https://reviews.llvm.org/D28037 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27872: [APFloat] Switch from (PPCDoubleDoubleImpl, IEEEdouble) layout to (IEEEdouble, IEEEdouble)

echristo added a comment. In https://reviews.llvm.org/D27872#628212, @timshen wrote: > I changed type style to early return. > > For constructors and destructors, I use: > > if (...) { > // statement; > return; > } > > > For normal functions that returns void, I chose: > > if (..

[PATCH] D27872: [APFloat] Switch from (PPCDoubleDoubleImpl, IEEEdouble) layout to (IEEEdouble, IEEEdouble)

echristo added a comment. In https://reviews.llvm.org/D27872#636147, @timshen wrote: > In https://reviews.llvm.org/D27872#636130, @echristo wrote: > > > Looks pretty weird. Typically I'd suggest just: > > > > if (foo) { > > > > Foo(); > > return; > > > > } > > > > since that will keep cogniti

[PATCH] D28409: Use CodegenOpts::less when creating a TargetMachine for clang `-O1`

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Sounds fine to me. I want to modify https://reviews.llvm.org/owners/package/1/ at some point, but there's nothing wrong with consistency for now. https://reviews.llvm.org/D28409 __

[PATCH] D27872: [APFloat] Switch from (PPCDoubleDoubleImpl, IEEEdouble) layout to (IEEEdouble, IEEEdouble)

echristo added a reviewer: scanon. echristo added a comment. Adding Steve in an attempt to get him to review :) https://reviews.llvm.org/D27872 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cf

[PATCH] D25435: Add -fdebug-info-for-profiling to emit more debug info for sample pgo profile collection

echristo added a comment. I don't think this ever was hashed out in the llvm-dev thread? https://reviews.llvm.org/D25435 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30415: Fix -mno-altivec cannot overwrite -maltivec option

echristo requested changes to this revision. echristo added a comment. This revision now requires changes to proceed. Different suggestion: Remove the faltivec option. Even gcc doesn't support it anymore afaict. (Go ahead and commit the zvector part if you'd like). -eric https://reviews.llvm.

[PATCH] D30415: Fix -mno-altivec cannot overwrite -maltivec option

echristo added a comment. In https://reviews.llvm.org/D30415#703652, @uweigand wrote: > In https://reviews.llvm.org/D30415#703442, @hfinkel wrote: > > > In https://reviews.llvm.org/D30415#703398, @echristo wrote: > > > > > Different suggestion: > > > > > > Remove the faltivec option. Even gcc doe

[PATCH] D30415: Fix -mno-altivec cannot overwrite -maltivec option

echristo added a comment. In https://reviews.llvm.org/D30415#705196, @uweigand wrote: > In https://reviews.llvm.org/D30415#704761, @echristo wrote: > > > In https://reviews.llvm.org/D30415#703652, @uweigand wrote: > > > > > I'm a bit confused by this discussion. -faltivec and -maltivec are > >

[PATCH] D30415: Fix -mno-altivec cannot overwrite -maltivec option

echristo added a comment. In https://reviews.llvm.org/D30415#705889, @echristo wrote: > In https://reviews.llvm.org/D30415#705196, @uweigand wrote: > > > In https://reviews.llvm.org/D30415#704761, @echristo wrote: > > > > > In https://reviews.llvm.org/D30415#703652, @uweigand wrote: > > > > > > >

[PATCH] D30760: Record command lines in objects built by clang

echristo requested changes to this revision. echristo added a comment. This revision now requires changes to proceed. Why aren't we passing the flags down as a string in the IR? Needs a testcase: you should only check for IR in clang tests. To make sure something is propagated to the other end u

[PATCH] D31213: Add support for -fno-auto-profile and -fno-profile-sample-use

echristo added a comment. Can you add a test for fno-auto-profile? https://reviews.llvm.org/D31213 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31213: Add support for -fno-auto-profile and -fno-profile-sample-use

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

[PATCH] D30415: Fix -mno-altivec cannot overwrite -maltivec option

echristo added a comment. In https://reviews.llvm.org/D30415#706370, @sfertile wrote: > > I have a patch to do this now. I'll plan on committing it in a bit. > > Is there a way to mark the -f form of the option as deprecated? We should > warn and suggest users switch to the -maltivec option for

[PATCH] D30760: Record command lines in objects built by clang, Clang part

echristo added a comment. Needs more testing. Might want to make sure that you actually are recording some useful command line options and that you're looking at the cc1 command line. This should also be a Driver test and not CodeGen. You can then use -### to inspect the command lines as passe

[PATCH] D30760: Record command lines in objects built by clang, Clang part

echristo added inline comments. Comment at: test/Driver/debug-options.c:201-202 // +// GRECORD: "-dwarf-debug-flags" +// GRECORD: -### -c -grecord-gcc-switches +// This seems a little light on the testing, would you mind adding some more interesting lines here?

[PATCH] D30760: Record command lines in objects built by clang, Clang part

echristo added inline comments. Comment at: test/Driver/debug-options.c:201-202 // +// GRECORD: "-dwarf-debug-flags" +// GRECORD: -### -c -grecord-gcc-switches +// george.burgess.iv wrote: > echristo wrote: > > This seems a little light on the testing, would you

[PATCH] D30760: Record command lines in objects built by clang, Clang part

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Sounds good. Do you have commit access? https://reviews.llvm.org/D30760 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.

[PATCH] D30760: Record command lines in objects built by clang, Clang part

echristo added a comment. Committed thusly: echristo@athyra ~/s/l/t/clang> git svn dcommit Committing to https://llvm.org/svn/llvm-project/cfe/trunk ... M lib/Driver/ToolChains/Clang.cpp M test/Driver/debug-options.c Committed r299037 https://reviews.llvm.org/D30760

[PATCH] D31440: PR32382: Adapt to LLVM changes in DIExpression.

echristo requested changes to this revision. echristo added a comment. This revision now requires changes to proceed. Sounds like Dave is asking for changes so I'll put it down as Request Changes to get it out of my queue. :) https://reviews.llvm.org/D31440 __

[PATCH] D25435: Add -fdebug-info-for-profiling to emit more debug info for sample pgo profile collection

echristo accepted this revision. echristo added a comment. Thanks for explaining all of this and going through it Dehao. LGTM. -eric https://reviews.llvm.org/D25435 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-

[PATCH] D28037: [Altivec] Change vec_sl to a << (b % (sizeof(a) * 8))

echristo added a comment. Going to commit this? https://reviews.llvm.org/D28037 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27872: [APFloat] Switch from (PPCDoubleDoubleImpl, IEEEdouble) layout to (IEEEdouble, IEEEdouble)

echristo added a comment. Few comments inline. Generally looks OK, I do share Hal's comment on finding some way of simplifying the if (someSemantics) ... if (otherSemantics) ... llvm_unreachable(...) pattern. What did you have in mind? Comment at: llvm/include/llvm/ADT/APFlo

<    1   2   3   >