[PATCH] D26546: [PPC] Add vec_insert4b/vec_extract4b to altivec.h

echristo closed this revision. echristo added a comment. This was committed: commit d65cd1f9424369c4ae7f945fac7fd9e4357451b2 Author: Sean Fertile Date: Thu Jan 5 21:43:30 2017 + Add vec_insert4b and vec_extract4b functions to altivec.h Add builtins for the

[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] D29770: [Assembler] Inline assembly diagnostics test.

echristo added a comment. Can we not get llc to use the diags interfaces here? https://reviews.llvm.org/D29770 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29770: [Assembler] Inline assembly diagnostics test.

echristo added a comment. In https://reviews.llvm.org/D29770#674961, @sanwou01 wrote: > Use clang_cc1 -verify for testing as suggested by @rnk. > > @echristo, llc does use the same diags interfaces as clang, however, it is > lacking the infrastructure to make use of LocCookies. > > In any case,

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

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. This looks fine to me now, might be good to get someone else to ack as well though. https://reviews.llvm.org/D27872 ___ cfe-commits mailing

[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

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

echristo added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:2743-2745 if (LinkageName == Name || (!CGM.getCodeGenOpts().EmitGcovArcs && !CGM.getCodeGenOpts().EmitGcovNotes && +

[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] D25435: Add -femit-accurate-debug-info to emit more debug info for sample pgo profile collection

echristo added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:2743-2745 if (LinkageName == Name || (!CGM.getCodeGenOpts().EmitGcovArcs && !CGM.getCodeGenOpts().EmitGcovNotes && +

[PATCH] D27066: Fix crash with unsupported architectures in Linux/Gnu target triples.

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. LGTM. Thanks! https://reviews.llvm.org/D27066 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[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

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

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

[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

[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

[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

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

[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

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

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

[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

[PATCH] D35131: Prevent ClangTools from generating dependency files.D34304 created a way for ToolInvocations to conditionally generatedependency files, and updated call sites to preserve the old behav

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. This sort of thing seems obvious in the future :) -eric https://reviews.llvm.org/D35131 ___ cfe-commits mailing list

[PATCH] D35519: Add SEMA checking to attribute 'target'

echristo added a comment. If you wouldn't mind I'd like to see this separated out a bit - a) I think we can make the attribute info a struct rather than a pair as a simple change first b) setCPU split c) let's see where we are after that? Also the description makes it sound like you're adding

[PATCH] D35449: [X86] Implement __builtin_cpu_is

echristo added a comment. From my perspective here once you and Erich get some agreement on the checking between your two bits I'm fine :) -eric https://reviews.llvm.org/D35449 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35449: [X86] Implement __builtin_cpu_is

echristo added a subscriber: erichkeane. echristo added a comment. Adding Erich Keane here on this since he's working on something similar for the target attribute. -eric https://reviews.llvm.org/D35449 ___ cfe-commits mailing list

[PATCH] D35449: [X86] Implement __builtin_cpu_is

echristo added a comment. FWIW the duplication in CGCall.cpp of the enum set is painful if you can come up with anything else it'd be awesome. I don't have any good ideas, just a fond wish :) https://reviews.llvm.org/D35449 ___ cfe-commits

[PATCH] D35572: Add isValidCPUName and isValidFeature to TargetInfo

echristo added a comment. Can you update the code in CGBuiltin to use this as part of this patch? :) Thanks! https://reviews.llvm.org/D35572 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35573: Improve SEMA for attribute-target

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. LGTM pending dependency approval https://reviews.llvm.org/D35573 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35574: Convert attribute 'target' parsing from a 'pair' to a 'struct' to make further improvements easier

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D35574 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35709: Remove Bitrig: CompilerRT Changes

echristo accepted this revision. echristo added a comment. LGTM. https://reviews.llvm.org/D35709 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35708: Remove Bitrig: Clang Changes

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D35708 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

echristo added a comment. So, what's the overall logical idea behind the need for this option? While I understand that you don't want people just doing this via the -mllvm set of options, but if you're just doing this to provide a temporary option rather than turning it off in the backend I'm

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

echristo added a comment. In https://reviews.llvm.org/D35577#817267, @sgundapa wrote: > The discussion is scattered across these patches > https://reviews.llvm.org/D35578 and https://reviews.llvm.org/D35579. > I will provide a brief summary here: > > The idea is to control the generation of

[PATCH] D35701: Break up Targets.cpp into a header/impl pair per target type[NFCI]

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. I'm going to say this ahead of time without looking into it "LGTM", but wait for ctopper (or someone else) to ack it for style etc since I'm unlikely to get to it any time shortly. :)

[PATCH] D35572: Add isValidCPUName and isValidFeature to TargetInfo

echristo added a comment. In https://reviews.llvm.org/D35572#813520, @erichkeane wrote: > In https://reviews.llvm.org/D35572#813369, @echristo wrote: > > > Can you update the code in CGBuiltin to use this as part of this patch? :) > > > > Thanks! > > > I'm not sure which you're referring to? I

[PATCH] D35572: Add isValidCPUName and isValidFeature to TargetInfo

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

[PATCH] D34425: Unified ARM logic for computing target ABI.

echristo added a comment. I've committed this for you as: M lib/Driver/ToolChains/Clang.cpp M test/CodeGen/arm-v8.1a-neon-intrinsics.c M test/CodeGen/named_reg_global.c M test/CodeGen/neon-immediate-ubsan.c M

[PATCH] D33820: [PowerPC] Pass CPU to assembler with -no-integrated-as

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Sorry, when I say "One inline comment otherwise LGTM" feel free to commit after fixing :) Since you have, then LGTM. -eric Repository: rL LLVM https://reviews.llvm.org/D33820

[PATCH] D34425: Unified ARM logic for computing target ABI.

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. This is OK once the dependent revision is approved. https://reviews.llvm.org/D34425 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33448: [CodeGen] Add thumb-mode to function target-features for arm/thumb triples.

echristo added a comment. In https://reviews.llvm.org/D33448#763411, @fhahn wrote: > In https://reviews.llvm.org/D33448#762410, @echristo wrote: > > > I probably would have added this as a feature in ARMTargetInfo similar to > > CRC/soft-float/etc. > > > > Thoughts? > > > Do you mean

[PATCH] D33448: [CodeGen] Add thumb-mode to target-features for arm/thumb triples.

echristo added a comment. In https://reviews.llvm.org/D33448#765749, @fhahn wrote: > I'll hold off merging this patch until https://reviews.llvm.org/D33436 lands, > which fixes a problem with mixed ARM/Thumb codegen OK. Commit at will :) -eric https://reviews.llvm.org/D33448

[PATCH] D33448: [CodeGen] Add thumb-mode to target-features for arm/thumb triples.

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. One minor nit and LGTM. Thanks! -eric Comment at: lib/Basic/Targets.cpp:5353 +// enable or disable thumb-mode per function +if (isThumb())

[PATCH] D33721: [ARM] Add support for target("arm") and target("thumb").

echristo added inline comments. Comment at: lib/Basic/Targets.cpp:5439-5442 +// [-|+]thumb-mode target features respectively. +std::vector UpdatedFeaturesVec(FeaturesVec); +for (auto : UpdatedFeaturesVec) { + if (Feature.compare("+arm") == 0)

[PATCH] D33721: [ARM] Add support for target("arm") and target("thumb").

echristo added inline comments. Comment at: include/clang/Basic/Attr.td:1790-1794 +// target features respectively. +if (Feature.compare("arm") == 0) + Ret.first.push_back("-thumb-mode"); +else if (Feature.compare("thumb") == 0) +

[PATCH] D34022: Repair 2010-05-31-palignr.c test

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Thanks for the fix! Do you need someone to commit it? https://reviews.llvm.org/D34022 ___ cfe-commits mailing list

[PATCH] D34022: Repair 2010-05-31-palignr.c test

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

[PATCH] D34304: Allow CompilerInvocations to generate .d files.

echristo edited reviewers, added: bkramer; removed: echristo. echristo added a comment. Going to let Ben review this. I'd rather not pass the bool down and he might know a way to avoid that here by knowing the code more :) https://reviews.llvm.org/D34304

[PATCH] D33820: [PowerPC] Pass CPU to assembler with -no-integrated-as

echristo added a comment. One inline comment otherwise LGTM Comment at: lib/Driver/ToolChains/Gnu.cpp:681 CmdArgs.push_back("-mppc"); -CmdArgs.push_back("-many"); +std::string CPU = getCPUName(Args, getToolChain().getTriple()); +

[PATCH] D33721: [ARM] Add support for target("arm") and target("thumb").

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Ah right. Thanks for looking. LGTM. -eric https://reviews.llvm.org/D33721 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D36431: Add powerpc64 to compiler-rt build infrastructure.

echristo accepted this revision. echristo added a comment. This is great with me. https://reviews.llvm.org/D36431 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37656: [cfi] Set function attributes for __cfi_* functions.

echristo added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:2917 +CodeGenFunction , llvm::Function *F, +bool ForceThumb) { + StringRef TargetCPU = CGF.getTarget().getTargetOpts().CPU;

[PATCH] D37656: [cfi] Set function attributes for __cfi_* functions.

echristo added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:2917 +CodeGenFunction , llvm::Function *F, +bool ForceThumb) { + StringRef TargetCPU = CGF.getTarget().getTargetOpts().CPU;

[PATCH] D38596: Implement attribute target multiversioning

echristo added a comment. This generally works for me modulo the things that Hal has mentioned. You'll probably want to add Richard to the review list for the Sema bits as well. Thanks! Comment at: lib/Sema/SemaDecl.cpp:3214 if (!isFriend &&

[PATCH] D38903: [ubsan] Only use indirect RTTI in prologues on Darwin

echristo added a comment. Given you were the last one in this code it seems reasonable to let you go for it :) That said, I didn't notice anything in particular that stuck out at me. https://reviews.llvm.org/D38903 ___ cfe-commits mailing list

[PATCH] D37428: Debug info: Fixed faulty debug locations for attributed statements

echristo added inline comments. Comment at: test/CodeGen/debug-info-attributed-stmt.c:1 +// RUN: %clang_cc1 -triple x86_64-unk-unk -disable-llvm-passes -debug-info-kind=limited -emit-llvm %s -o - | FileCheck %s + Since we're not optimizing or generating code

[PATCH] D36336: [X86] Add support for __builtin_cpu_init

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. One inline comment, but go ahead and commit after fixing that up. Comment at: lib/CodeGen/CGBuiltin.cpp:7292 const CallExpr

[PATCH] D36057: Use class to pass information about executable name

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Fine with me. -eric https://reviews.llvm.org/D36057 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D39179: [AArch64] Fix PR34625 -mtune without -mcpu should not set -target-cpu

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. This should be fine for now. Thanks! -eric https://reviews.llvm.org/D39179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[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] D46791: Make -gsplit-dwarf generally available

echristo added subscribers: pcc, paulsemel. echristo added a comment. FWIW Peter has some patches to move object emission away from objcopy that I'm on the hook to review here shortly so the objcopy part of this should become unnecessary and can just have us able to emit dwarf5 compatible split

[PATCH] D47070: [CUDA] Upgrade linked bitcode to enable inlining

echristo added a comment. In https://reviews.llvm.org/D47070#1105533, @Hahnfeld wrote: > Looks like this was added as a "temporary solution" in > https://reviews.llvm.org/D8984. Meanwhile the attribute whitelist was merged > half a year later (https://reviews.llvm.org/D7802), so maybe we can

[PATCH] D45783: [DEBUGINFO, NVPTX] Render `-no-cuda-debug` LLVM option when required.

echristo added a comment. So, I'd really prefer not to set options via the backend option path. From here I think we should aim to take all of the options we added and having the asm printer in the backend know how to set them depending on the target. We could also add things to the IR

[PATCH] D47029: [X86] Remove some preprocessor feature checks from intrinsic headers

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

[PATCH] D46541: [CodeGen] Improve diagnostics related to target attributes

echristo added a comment. I think this will work, one inline comment. Might also be good to get a few different test cases, e.g. one where we're not seeing the alphabetically first as the minimum :) Comment at: lib/CodeGen/CodeGenModule.h:1085 +

[PATCH] D46791: Make -gsplit-dwarf generally available

echristo added a comment. In https://reviews.llvm.org/D46791#1107168, @pcc wrote: > There were a bunch of them but the last one was > https://reviews.llvm.org/D47093 which has already landed :) > > Probably all that needs to happen on this change is to replace the isLinux() > check with

[PATCH] D46230: For x86_64, gcc 7.2 under Amazon Linux AMI sets its paths to x86_64-amazon-linux

echristo added a comment. Well, that's ridiculous. We should really fix this a better way in the future. That said, would you add a testcase for this please so we don't regress if we fix it? :) -eric Repository: rC Clang https://reviews.llvm.org/D46230

[PATCH] D47050: MC: Change the streamer ctors to take an object writer instead of a stream. NFCI.

echristo added a comment. LGTM. Repository: rC Clang https://reviews.llvm.org/D47050 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48100: Append new attributes to the end of an AttributeList.

echristo added subscribers: dlj, echristo. echristo added a comment. I've added a couple of inline comments here - between this and the comments in the post-commit review from dlj it seems like we might want to revert this for now and figure out the best way forward. Thanks! -eric

[PATCH] D46410: [Target] Diagnose mismatch in required CPU for always_inline functions

echristo added a comment. I like the idea of a front end warning, but had believed that a subset of cpu features should be allowed to be inlined into something that's a superset and it sounds like you don't agree? Your thoughts here? -eric https://reviews.llvm.org/D46410

[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

[PATCH] D41837: Add Function multiversion to the release notes.

echristo added a comment. I think you're missing that right now it's x86 only yes? :) -eric Repository: rC Clang https://reviews.llvm.org/D41837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D42227: [Refactor] Use enum instead of magic number in handleX86ForceAlignArgPointerAttr, NFC

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. LGTM. Let me know if you need this committed. Repository: rC Clang https://reviews.llvm.org/D42227 ___ cfe-commits mailing list

[PATCH] D43045: Add NVPTX Support to ValidCPUList (enabling march notes)

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. LGTM with an inline comment. Comment at: include/clang/Basic/Cuda.h:49 SM_72, + LAST, }; We have last, invalid, etc... maybe we should pick one

[PATCH] D43057: Add Rest of Targets Support to ValidCPUList (enabling march notes)

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. I've glanced at this and didn't notice anything. I think any problems can be fixed in post-commit review if necessary. https://reviews.llvm.org/D43057

[PATCH] D43095: Make attribute-target on a Definition-after-use update the LLVM attributes

echristo added inline comments. Comment at: lib/CodeGen/CodeGenModule.cpp:1318-1325 +llvm::AttrBuilder Attrs; +if (GetCPUAndFeaturesAttributes(D, Attrs)) { + // We know that GetCPUAndFeaturesAttributes will always have the + // newest set, since

[PATCH] D43095: Make attribute-target on a Definition-after-use update the LLVM attributes

echristo added inline comments. Comment at: lib/CodeGen/CodeGenModule.cpp:1318-1325 +llvm::AttrBuilder Attrs; +if (GetCPUAndFeaturesAttributes(D, Attrs)) { + // We know that GetCPUAndFeaturesAttributes will always have the + // newest set, since

[PATCH] D43359: Clean up 'target' attribute diagnostics

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Seems ok here. https://reviews.llvm.org/D43359 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43362: Simplify setting dso_local. NFC.

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. LGTM. Thanks for the cleanup. https://reviews.llvm.org/D43362 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43514: Start settinng dso_local for COFF

echristo added a comment. Some inline comments - I think this looks ok in general, but I'm curious about the answers to the questions/documentation bits inline. Thanks! Comment at: lib/CodeGen/CGDecl.cpp:257 + setGVProperties(GV, ); + If positioning is

[PATCH] D43514: Start settinng dso_local for COFF

echristo added inline comments. Comment at: lib/CodeGen/CodeGenModule.h:728 + /// This must be called after dllimport/dllexport is set. + /// FIXME: should this set dllimport/dllexport instead? void setGVProperties(llvm::GlobalValue *GV, const NamedDecl *D) const;

[PATCH] D40819: Implement Attribute Target MultiVersioning (Improved edition!)

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Couple of inline comments, otherwise I'm pretty happy. I'd wait for an ack by Richard for this though. -eric Comment at: lib/CodeGen/CGBuiltin.cpp:7673 -Value

[PATCH] D49930: [DebugInfo][OpenCL] Generate correct block literal debug info for OpenCL

echristo added a comment. The patch is fine, in general, couple of comments: a) Can you refactor this so the if conditionals are just two functions? Those functions are big enough already. b) I'm not quite sure why you're picking limited here, do you have an explanation? c) Can you split that

[PATCH] D49930: [DebugInfo][OpenCL] Generate correct block literal debug info for OpenCL

echristo added a comment. In https://reviews.llvm.org/D49930#1181000, @scott.linder wrote: > Sorry, I didn't see the additional comments until after I committed. I will > make those changes; is it OK to update this review, or should I create a new > one? A new one is great. Just treat this

[PATCH] D49828: [libc++] Add hack to allow ubsan to work w/o compiler-rt (__muloti4 is undefined)

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Bit of a hack, but I'm ok with it. Repository: rCXX libc++ https://reviews.llvm.org/D49828 ___ cfe-commits mailing list

[PATCH] D50099: [DebugInfo][OpenCL] Address post-commit review of D49930

echristo added a comment. Looks pretty good. Could you pass CGM in and just make the functions static I couldn't see any other class variables, but might have missed something. One inline comment as well. -eric Comment at: lib/CodeGen/CGDebugInfo.cpp:997

[PATCH] D49148: [DEBUGINFO] Disable unsupported debug info options for NVPTX target.

echristo added a comment. In https://reviews.llvm.org/D49148#1184957, @tra wrote: > I wonder, what's the right thing to do to silence the warnings. For instance, > we compile everything with -Werror and the warnings result in build breaks. > > Easy way out is to pass

[PATCH] D50099: [DebugInfo][OpenCL] Address post-commit review of D49930

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

[PATCH] D50099: [DebugInfo][OpenCL] Address post-commit review of D49930

echristo added a comment. In https://reviews.llvm.org/D50099#1189667, @scott.linder wrote: > When I went to mark these as static I noticed they use > `CGDebugInfo::CreateMemberType` which uses a couple other non-static member > functions, and it starts to become difficult to tease things out

[PATCH] D51177: [DEBUGINFO] Add support for emission of the debug directives only.

echristo added a comment. Should we just have them mean the same thing and change it based on target? Repository: rC Clang https://reviews.llvm.org/D51177 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D51007: Test the cross-product of options that affect how libgcc-related arguments are passed to the linker.

echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Feel free to add any tests like this that test existing behavior without review in the future. If we want to change the behavior we should probably review that though :) -eric

  1   2   3   >