[PATCH] D138247: PR58819: Correct linkage and mangling of lambdas in inline static member initializers

efriedma added a comment. The relevant text of the current Itanium ABI (which was updated in https://github.com/itanium-cxx-abi/cxx-abi/commit/d8e9d102c83f177970f0db6cc8bee170f2779bc1) > In the following contexts, however, the one-definition rule requires closure > types in different translatio

[PATCH] D146466: [clang] diagnose function fallthrough

efriedma added a comment. There are limits to how much we can do by just changing the code clang generates... but the two particular cases you mention probably could be "fixed" by messing with the IR generated by clang. Sure, that probably makes sense to pursue. (If you're going to pick an ar

[PATCH] D146466: [clang] diagnose function fallthrough

efriedma added a comment. The problem with a change like that is that it's not clear what the underlying semantic model is. If we add a flag that says "try to generate code that matches Linux kernel developers' mental model of the underlying machine", or "loop unrolling should try to preserve

[PATCH] D146466: [clang] diagnose function fallthrough

efriedma added a comment. I think the reason "recoverable" ubsan causes trouble is that it introduces branches that subsequent optimizations can abuse. So without ubsan, we just have an udiv instruction. With ubsan, we conveniently have a branch on exactly the condition that would make the ud

[PATCH] D146386: [MS ABI] Fix mangling references to declarations.

efriedma added a comment. > I don't still understand how to mangle nested unnamed tags in general. According to some quick experiments, for the non-virtual case, you mangle a member of an unnamed union it the same way as a regular member, except you stick `@` into the mangling. Additional leve

[PATCH] D146386: [MS ABI] Fix mangling references to declarations.

efriedma added a comment. The numbers are backreferences of the sort generated by mangleSourceName(), I think. If you nest deeply enough, MSVC stops using them; for example: struct A { union {union { union {union { struct B {struct C {struct D {struct E {struct F {struct G { struct H {s

[PATCH] D144889: [C2x] Support in freestanding

efriedma requested changes to this revision. efriedma added a comment. This revision now requires changes to proceed. Providing this header doesn't do anything useful without an actual implementation; all of these "__builtin" calls just lower to libc calls in the general case. How do you plan t

[PATCH] D144889: [C2x] Support in freestanding

efriedma added a comment. Historically, the required functions for a "freestanding" C implementation were very restricted. Freestanding headers didn't export library functions, just constants and types. As a practical matter, we actually do need a few functions to support code generation for

[PATCH] D144889: [C2x] Support in freestanding

efriedma added a comment. In D144889#4156120 , @bcraig wrote: > A freestanding implementation doesn't necessarily mean that everything is > header-only. It's fine to require linking against a (freestanding) C runtime > library. All header-only is fine

[PATCH] D144889: [C2x] Support in freestanding

efriedma added a comment. > perhaps because they did char buf[256] = {0} though Right, we need memcpy and memset to emit reasonable code for struct/array initialization and assignment. We don't provide any library that includes memcpy/memset/memmove because of a combination of legacy, and that

[PATCH] D143233: [Clang][CodeGen] Fix this argument type for certain destructors

efriedma closed this revision. efriedma added a comment. rG674099113533 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143233/new/ https://reviews.llvm.org/D143233 _

[PATCH] D143233: [Clang][CodeGen] Fix this argument type for certain destructors

efriedma added a comment. Maybe worth cherry-picking to 16 branch? I think someone will need to rebase onto the branch for that, though; there was merge conflict on the microsoft-abi-eh-cleanups.cpp change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D145416: [clang] model 'p' inline asm constraint as reading memory

efriedma added a comment. I think this is going to change what inputs Sema will accept for "p". If that's intentional, please add test coverage. Otherwise, please make a narrower change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145416/new/

[PATCH] D145564: [clang][docs] Clarify the semantics of -fexceptions

efriedma added a comment. I'd prefer to focus the documentation more generically on exceptions, not unwind information specifically. -fexceptions makes code generation change in other ways, and "unwind information" doesn't exist on all targets where exception handling is relevant. CHANGES SI

[PATCH] D145564: [clang][docs] Clarify the semantics of -fexceptions

efriedma added a comment. Something like that, sure. Is it actually on by default on x86-64? I don't think the default is target-specific, I think it's just on for default for C++ dialects, and off by default for non-C++ dialects. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145564/

[PATCH] D144903: [X86] Drop single use check for freeze(undef) in LowerAVXCONCAT_VECTORS

efriedma added a comment. I think the point of the hasOneUse check is to avoid a possible miscompile; if a FREEZE has more than one use, all users need to see the same value. So not sure dropping the check is correct in general. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D145726: Fix assembler error when -g and -gdwarf-* is passed with -fno-integrated-as.

efriedma added a comment. Following reproduces for me (clang from main, Ubuntu 16.04). $ cat test.cpp int foo() { int i=6; do --i; while (!(i%3)); do {} while (!(i%5)); return 0; } $ clang++ test.cpp -c -fno-integrated-as -gdwarf-4 -O2 -fno-finite

[PATCH] D145369: Emit const globals with constexpr destructor as constant LLVM values

efriedma added a comment. There appear to be 8 different places that call `isTypeConstant(Ty, true)`; I'd like to see testcases for all of them if it's relevant. (I agree with your assessment for global variables, but not sure about the other places.) Repository: rG LLVM Github Monorepo CH

[PATCH] D145726: Fix assembler error when -g and -gdwarf-* is passed with -fno-integrated-as.

efriedma added a comment. In D145726#4190341 , @garvitgupta08 wrote: > In D145726#4190316 , > @nickdesaulniers wrote: > >> Isn't this only an issue with ancient versions of GNU as? Older than 2.16? > > I am able

[PATCH] D153179: [clang codegen] Fix ABI for HVA returns on AArch64 MSVC.

efriedma added a comment. Filed https://github.com/llvm/llvm-project/issues/63417 . (On a related note, I also filed https://github.com/llvm/llvm-project/issues/63360 for an issue I stumbled over involving deleted copy constructors.) > Interesting. In Clang, we basically layer the C++ rules ov

[PATCH] D153179: [clang codegen] Fix ABI for HVA returns on AArch64 MSVC.

efriedma updated this revision to Diff 533045. efriedma added a comment. Updated testcase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153179/new/ https://reviews.llvm.org/D153179 Files: clang/lib/CodeGen/MicrosoftCXXABI.cpp clang/test/CodeG

[PATCH] D153179: [clang codegen] Fix ABI for HVA returns on AArch64 MSVC.

efriedma updated this revision to Diff 533049. efriedma added a comment. Fix typo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153179/new/ https://reviews.llvm.org/D153179 Files: clang/lib/CodeGen/MicrosoftCXXABI.cpp clang/test/CodeGenCXX/hom

[PATCH] D153179: [clang codegen] Fix ABI for HVA returns on AArch64 MSVC.

efriedma updated this revision to Diff 533050. efriedma added a comment. Upload correct diff. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153179/new/ https://reviews.llvm.org/D153179 Files: clang/lib/CodeGen/MicrosoftCXXABI.cpp clang/test/Co

[PATCH] D143241: [Clang] Reset FP options before function instantiations

efriedma accepted this revision. efriedma 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/D143241/new/ https://reviews.llvm.org/D143241 ___

[PATCH] D153419: Enable fexec-charset option

efriedma added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6864 + CmdArgs.push_back("-fexec-charset"); + CmdArgs.push_back(Args.MakeArgString(Triple.getSystemCharset())); if (Arg *execCharset = Args.getLastArg(options::OPT_fexec_charset_EQ)) {

[PATCH] D153417: New CharSetConverter wrapper class for ConverterEBCDIC

efriedma added a comment. As I mentioned at https://discourse.llvm.org/t/rfc-enabling-fexec-charset-support-to-llvm-and-clang-reposting/71512 , I think SimplifyLibCalls needs to be aware of encodings. To make that work, this probably needs to be somewhere in llvm/ , not clang/ . Repository:

[PATCH] D153418: Adding iconv support to CharSetConverter class

efriedma added a comment. This doesn't really address the concerns from https://discourse.llvm.org/t/rfc-adding-a-charset-converter-to-the-llvm-support-library/69795/17 about consistency. It's bad if different hosts support a different set of charsets/charset names, and it's really bad if a gi

[PATCH] D153418: Adding iconv support to CharSetConverter class

efriedma added a comment. Even if we do decide we have to use platform-specific facilities because there's no suitable library, I think we should at least have a hardcoded set of encodings we recognize, so we aren't passing arbitrary encoding names directly from the command-line to the iconv()

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1279 + if (isa(E)) +return nullptr; + nickdesaulniers wrote: > efriedma wrote: > > nickdesaulniers wrote: > > > efriedma wrote: > > > > efriedma wrote: > > > > > This ne

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8360 + // Do not constant fold an R-value. + if (Info.EvalMode == EvalInfo::EM_ConstantFold && !E->isLValue()) +return false; Checking isLValue() doesn't make sense; consider: ``` s

[PATCH] D153560: [Clang] Allow C++11 style initialisation of SVE types.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1878 + +llvm_unreachable("Unexpected initialization of a scalable vector!"); + } I can see why you can't have more than one element... but both zero and one seem feasible. (For

[PATCH] D153560: [Clang] Allow C++11 style initialisation of SVE types.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1878 + +llvm_unreachable("Unexpected initialization of a scalable vector!"); + } paulwalker-arm wrote: > efriedma wrote: > > I can see why you can't have more than one element...

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/test/CodeGenOpenCL/amdgpu-nullptr.cl:60 -// CHECK: @fold_priv ={{.*}} local_unnamed_addr addrspace(1) global ptr addrspace(5) addrspacecast (ptr null to ptr addrspace(5)), align 4 +// CHECK: @fold_priv ={{.*}} local_unnamed_add

[PATCH] D153179: [clang codegen] Fix ABI for HVA returns on AArch64 MSVC.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa1540e4852a9: [clang codegen] Fix ABI for HVA returns on AArch64 MSVC. (authored by efriedma). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D153417: New CharSetConverter wrapper class for ConverterEBCDIC

efriedma added a comment. In D153417#4449812 , @abhina.sreeskantharajan wrote: > In D153417#4438764 , @efriedma > wrote: > >> As I mentioned at >> https://discourse.llvm.org/t/rfc-enabling-fexec-charset-support

[PATCH] D153418: Adding iconv support to CharSetConverter class

efriedma added a comment. > I think this is reasonable since gcc's fexec-charset option also says the > name can be any encoding supported by the iconv library (copy pasted below > from https://gcc.gnu.org/onlinedocs/gcc-13.1.0/gcc.pdf) so this would match > that behaviour "gcc did it" doesn't

[PATCH] D153560: [Clang] Allow C++11 style initialisation of SVE types.

efriedma accepted this revision. efriedma 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/D153560/new/ https://reviews.llvm.org/D153560 ___

[PATCH] D154007: Reland "Try to implement lambdas with inalloca parameters by forwarding without use of inallocas."

efriedma added a comment. I'm not confident that isUsed() works the way you want it to in this context. In particular, if the code in question runs before we've translated the whole translation unit, the isUsed() bit could change. If you want that's more obviously safe, you could just check i

[PATCH] D154043: [CodeGen] -fsanitize={function, kcfi}: ensure align 4 if +strict-align

efriedma added a comment. I also think it makes sense to fix the alignment when we lower the metadata, not in the frontend, unless I'm missing something. It's not clear to me how "strict-align" is relevant; if sanitizer lowering is generating "align 4" loads, the relevant pointers need to be ap

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

efriedma added a comment. The fundamental problem here is the interaction with SFINAE; if we don't diagnose this as an error, we actually miscompile some cases. Because of that, a flag wouldn't really be "turning off the warning"; it would be enabling a non-compliant language mode that has dif

[PATCH] D138247: PR58819: Correct linkage and mangling of lambdas in inline static member initializers

efriedma accepted this revision as: efriedma. efriedma 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/D138247/new/ https://reviews.llvm.org/D138247 __

[PATCH] D147307: [clang] Consider artificial always inline builtin as inline builtins

efriedma added a comment. I'd be fine with just dropping the check for GNUInline. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147307/new/ https://reviews.llvm.org/D147307 ___ cfe-commits mailing list c

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

efriedma added a subscriber: nickdesaulniers. efriedma added a comment. Any thoughts on diagnostics here? If I'm not mistaken, with this patch, if you request an impossible tail call, you get a crash with very little useful information. (Although, see https://discourse.llvm.org/t/rfc-improvin

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

efriedma added a comment. An error message pointing at the call in the source code would be good enough, I think. Adding a "reason" might be nice, but not strictly necessary. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147714/new/ https://reviews.llvm.org/D147714 __

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

efriedma added a comment. I think the issue is that you're calling EmitPointerWithAlignment() on the argument, then calling EmitScalarExpr on the same argument. Essentially, emitting the argument twice. If emitting the argument has side-effects, that will cause an issue. Sorry, should have s

[PATCH] D147867: [Windows SEH] Fix ehcleanup crash for Windows -EHa

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/test/CodeGen/windows-seh-EHa-CppCatchReturn.cpp:27 + +// FIXME: We may need to generate llvm.seh.scope.end or remove llvm.seh.scope.begin. +void q

[PATCH] D147307: [clang] Do not require GNUInlineAttr for inline builtins

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147307/new/ https://reviews.llvm.org/D147307 ___ cfe-commits mailing list cfe-commi

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

efriedma added a comment. Please don't upload reviews for followup patches on top of the original patch; it confuses the history of happened when. But sure, looks fine. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143813/new/ https://reviews.llvm.org/D143813

[PATCH] D148723: [clang] Fix comdat for InlineBuiltin declarations

efriedma added a comment. I spoke a little loosely when I said "C++ code". I meant "code using C++ 'inline' semantics", which includes C code on Windows. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148723/new/ https://reviews.llvm.org/D148723 ___

[PATCH] D137872: Implement lambdas with inalloca parameters by forwarding to function without inalloca calling convention.

efriedma added a comment. In D137872#4348314 , @akhuang wrote: > In D137872#4327615 , @efriedma > wrote: > >> I'm having a bit of trouble following how exactly the thunk creation is >> working here... do we gene

[PATCH] D150892: [clang][ExprConstant] fix __builtin_object_size for flexible array members

efriedma added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:11740 + LVal.getLValueBase().dyn_cast()); + Result += VD->getFlexibleArrayInitChars(Info.Ctx); +} nickdesaulniers wrote: > nickdesaulniers wrote: > > erichkeane wrote: >

[PATCH] D150892: [clang][ExprConstant] fix __builtin_object_size for flexible array members

efriedma added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:11740 + LVal.getLValueBase().dyn_cast()); + Result += VD->getFlexibleArrayInitChars(Info.Ctx); +} nickdesaulniers wrote: > efriedma wrote: > > nickdesaulniers wrote: > >

[PATCH] D123649: Allow flexible array initialization in C++.

efriedma added a comment. The assertion is correct; without it, your code is getting miscompiled. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123649/new/ https://reviews.llvm.org/D123649 ___ cfe-commit

[PATCH] D151148: [clang][ExprConstant] fix __builtin_object_size for compound literal

efriedma added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:11737 bool Ret = HandleSizeof(Info, ExprLoc, Ty, Result); if (Ty->isStructureType() && Ty->getAsStructureType()->getDecl()->hasFlexibleArrayMember()) { For the second ca

[PATCH] D123649: Allow flexible array initialization in C++.

efriedma added a comment. In D123649#4362418 , @aaron.ballman wrote: > In D123649#4362402 , @efriedma > wrote: > >> The assertion is correct; without it, your code is getting miscompiled. > > The assertion may b

[PATCH] D151148: [clang][ExprConstant] fix __builtin_object_size for compound literal

efriedma added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:11737 bool Ret = HandleSizeof(Info, ExprLoc, Ty, Result); if (Ty->isStructureType() && Ty->getAsStructureType()->getDecl()->hasFlexibleArrayMember()) { nickdesaulniers w

[PATCH] D151172: [CodeGen] Fix the type of the constant that is used to zero-initialize a flexible array member

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:964 + if (NonzeroLength == 0 && + (DesiredType->getNumElements() != 0 || Elements.empty())) return llvm::ConstantAggregateZero::get(DesiredType); Not sure I like this fix.

[PATCH] D150892: Reland: [clang][ExprConstant] fix __builtin_object_size for flexible array members

efriedma accepted this revision as: efriedma. efriedma added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150892/new/ https://reviews.llvm.org/D150892 ___ cfe-commits mailing list cfe-co

[PATCH] D151172: [CodeGen] Fix the type of the constant that is used to zero-initialize a flexible array member

efriedma accepted this revision. efriedma 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/D151172/new/ https://reviews.llvm.org/D151172 ___

[PATCH] D76096: [clang] allow const structs to be constant expressions in initializer lists

efriedma added a comment. > It makes sense to me that const int foo[] = [ ...massive list...]; would take > long to validate the entire initializer as all constant expressions The expensive part we're currently avoiding by bailing out of the constant evaluator (the code D76169

[PATCH] D151393: [CodeGen] Make __clang_call_terminate have an unwind table entry

efriedma added inline comments. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4698 +if (CodeGenOpts.UnwindTables) + fn->setUWTableKind(llvm::UWTableKind(CodeGenOpts.UnwindTables)); + We probably want to call SetLLVMFunctionAttributesForDefinition() her

[PATCH] D151393: [CodeGen] Make __clang_call_terminate have an unwind table entry

efriedma accepted this revision. efriedma added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151393/new/ https://reviews.llvm.org/D151393 ___ cfe-commits mailing list cfe-commits@lists.l

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

efriedma added a comment. I think I'm okay with the semantics as-is. Comment at: clang/include/clang/Driver/Options.td:1703 + PosFlag, NegFlag, + BothFlags<[NoXarchOption], " static variables if unused">>; defm fixed_point : BoolFOption<"fixed-point", Can yo

[PATCH] D148723: [clang] Restrict Inline Builtin to non-static, non-odr linkage

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148723/new/ https://reviews.llvm.org/D148723 ___ cfe-commits mailing list cfe-commi

[PATCH] D151515: [CodeGen] add additional cast when checking call arguments

efriedma added a comment. Relevant bit of the AST: `-ExprWithCleanups 0xd16a780 'void':'void' `-CXXOperatorCallExpr 0xd16a678 'void':'void' '()' |-ImplicitCastExpr 0xd16a5a8 'void (*)(int (&&)[]) const' | `-DeclRefExpr 0xd16a528 'void (int (&&)[]) const' lvalue CXXMethod

[PATCH] D151572: [clang][ConstantEmitter] have tryEmitPrivate try ConstExprEmitter fast-path first

efriedma added a comment. Did you intentionally skip moving the ConstExprEmitter call in tryEmitPrivateForVarInit? (VarDecl::evaluateValue calls the constant evaluator.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151572/new/ https://reviews.l

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324 // This is a string literal initializing an array in an initializer. -return CGM.GetConstantArrayFromStringLiteral(E); +return E->isLValue() ? + CGM.GetAddrOfConstantStringFrom

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324 // This is a string literal initializing an array in an initializer. -return CGM.GetConstantArrayFromStringLiteral(E); +return E->isLValue() ? + CGM.GetAddrOfConstantStringFrom

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324 // This is a string literal initializing an array in an initializer. -return CGM.GetConstantArrayFromStringLiteral(E); +return E->isLValue() ? + CGM.GetAddrOfConstantStringFrom

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324 // This is a string literal initializing an array in an initializer. -return CGM.GetConstantArrayFromStringLiteral(E); +return E->isLValue() ? + CGM.GetAddrOfConstantStringFrom

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1209 +return VisitCastExpr(I, T); + } + llvm::Constant *VisitCXXFunctionalCastExpr(CXXFunctionalCastExpr *C, QualType T) { ConstExprEmitter should inherit an identical impleme

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

efriedma added reviewers: efriedma, rsmith. efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324 // This is a string literal initializing an array in an initializer. -return CGM.GetConstantArrayFromStringLiteral(E); +return E->isLValu

[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324 // This is a string literal initializing an array in an initializer. -return CGM.GetConstantArrayFromStringLiteral(E); +return E->isLValue() ? + CGM.GetAddrOfConstantStringFrom

[PATCH] D143386: Add function pointer alignment to DataLayout

efriedma added a comment. In D143386#4110398 , @gchatelet wrote: >> And the x86 doesn't specify 4-byte alignment. > > I thought that specifying it in the `DataLayout` would help getting rid of > the special handling in ConstantFold >

[PATCH] D142584: [CodeGen] Add a boolean flag to `Address::getPointer` and `Lvalue::getPointer` that indicates whether the pointer is known not to be null

efriedma added a comment. The approach here makes sense. Comment at: clang/lib/CodeGen/Address.h:65 : Pointer(Pointer), ElementType(ElementType) { if (Alignment.isZero()) return; Do you need to do something with IsKnownNonNull in the `Alignme

[PATCH] D139741: [clang][CodeGen] Use base subobject type layout for potentially-overlapping fields

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:370 + const auto StorageAlignment = getAlignment(StorageType); + if (LayoutSize % StorageAlignment || Layout.getDataSize() % StorageAlignment) Packed = true; dzhidzhoev

[PATCH] D137980: [ARM] Pretend atomics are always lock-free for small widths.

efriedma added a comment. Herald added a subscriber: StephenFan. Any further comment on this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137980/new/ https://reviews.llvm.org/D137980 ___ cfe-commits ma

[PATCH] D140123: [TLS] Clamp the alignment of TLS global variables if required by the target

efriedma added a comment. Missing LangRef change? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140123/new/ https://reviews.llvm.org/D140123 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https:

[PATCH] D143919: [Clang] Copy strictfp attribute from pattern to instantiation

efriedma added a comment. We have code somewhere to generically copy attributes from function templates to instantiations, right? Why do we need to explicitly copy StrictFPAttr in particular, separate from that? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D140123: [TLS] Clamp the alignment of TLS global variables if required by the target

efriedma added a comment. In D140123#4113536 , @efriedma wrote: > Missing LangRef change? Ping? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140123/new/ https://reviews.llvm.org/D140123

[PATCH] D143233: [Clang][CodeGen] Fix this argument type for certain destructors

efriedma added a comment. If the parameter attributes are relevant, should we change the CHECK lines in the tests to check for them? At least for some of the tests. Once we fix these tests to enable opaque pointer types, you won't actually be checking for anything. Repository: rG LLVM Git

[PATCH] D143233: [Clang][CodeGen] Fix this argument type for certain destructors

efriedma accepted this revision. efriedma 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/D143233/new/ https://reviews.llvm.org/D143233 ___

[PATCH] D140123: [TLS] Clamp the alignment of TLS global variables if required by the target

efriedma added a comment. > no separate section for module flags https://llvm.org/docs/LangRef.html#module-flags-metadata ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140123/new/ https://reviews.llvm.org/D140123 ___

[PATCH] D140123: [TLS] Clamp the alignment of TLS global variables if required by the target

efriedma added a comment. There are subsections for individual flags. Maybe we could mess with the markup to make that a bit more clear... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140123/new/ https://reviews.llvm.org/D140123 ___

[PATCH] D142584: [CodeGen] Add a boolean flag to `Address::getPointer` and `Lvalue::getPointer` that indicates whether the pointer is known not to be null

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/CodeGen/Address.h:67 return; -// Currently the max supported alignment is much less than 1 << 63 and is +// Currently the max su

[PATCH] D143919: [Clang] Copy strictfp attribute from pattern to instantiation

efriedma added a comment. If the "strictfp" attribute is based on the contents of the function body, should we recompute it when the function is instantiated, as opposed to copying it? For example, say you have a pragma inside an "if constexpr"; should that set the strictfp attribute if it's d

[PATCH] D142584: [CodeGen] Add a boolean flag to `Address::getPointer` and `Lvalue::getPointer` that indicates whether the pointer is known not to be null

efriedma added inline comments. Comment at: clang/lib/CodeGen/Address.h:67 return; -// Currently the max supported alignment is much less than 1 << 63 and is +// Currently the max supported alignment is much less than 1 << 32 and is // guaranteed to be a power

[PATCH] D143919: [Clang] Copy strictfp attribute from pattern to instantiation

efriedma added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:824 + continue; +} + sepavloff wrote: > efriedma wrote: > > Is this necessary? The non-delayed-parsed case seems to work correctly on > > trunk without any chang

[PATCH] D139741: [clang][CodeGen] Use base subobject type layout for potentially-overlapping fields

efriedma accepted this revision. efriedma added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139741/new/ https://reviews.llvm.org/D139741 ___ cfe-commits mailing list cfe-commits@lists.l

[PATCH] D143919: [Clang] Copy strictfp attribute from pattern to instantiation

efriedma accepted this revision. efriedma 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/D143919/new/ https://reviews.llvm.org/D143919 ___

[PATCH] D143233: [Clang][CodeGen] Fix this argument type for certain destructors

efriedma added a comment. How do you want to be listed as author in the git commit (Name )? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143233/new/ https://reviews.llvm.org/D143233 ___ cfe-commits mail

[PATCH] D147875: [clang][Diagnostics] Show line numbers when printing code snippets

efriedma added inline comments. Comment at: clang/lib/Frontend/TextDiagnostic.cpp:1121-1138 +static unsigned getNumDisplayWidth(unsigned N) { + if (N < 10) +return 1; + if (N < 100) +return 2; + if (N < 1'000) +return 3; aaron.ballman wrote: > jrtc

[PATCH] D148092: [clang][CodeGen] Break up TargetInfo.cpp [4/6]

efriedma accepted this revision. efriedma 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/D148092/new/ https://reviews.llvm.org/D148092 ___

[PATCH] D137872: Implement lambdas with inalloca parameters by forwarding to function without inalloca calling convention.

efriedma added a comment. I'm having a bit of trouble following how exactly the thunk creation is working here... do we generate different code depending on whether the call operator and/or the static invoker are referenced? Why is the function in EmitLambdaInAllocaCallOpFn not getting defined

[PATCH] D150192: Allow clang to emit inrange metadata when generating code for array subscripts

efriedma added a comment. From what I recall, "inrange" is actually more restrictive than the normal C/C++ array indexing rules. Specifically, the bits regarding comparisons. "inrange" was designed to allow splitting globals indexed using inrange. That isn't to say that functionality like this

[PATCH] D150192: Allow clang to emit inrange metadata when generating code for array subscripts

efriedma added a comment. See D115274 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150192/new/ https://reviews.llvm.org/D150192 ___ cfe-commits mailing list cfe-commit

[PATCH] D150340: [SEH]:Fix assertion when try is used inside catch(...) block with /EHa

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGException.cpp:650 +llvm::FunctionCallee SehCppScope = getSehTryBeginFn(CGM); +EmitSehScope(SehCppScope); + } Do we need to make the same change in EmitSEHTryStmt/ExitSEHTryStmt?

[PATCH] D150340: [SEH]:Fix assertion when try is used inside catch(...) block with /EHa

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGException.cpp:650 +llvm::FunctionCallee SehCppScope = getSehTryBeginFn(CGM); +EmitSehScope(SehCppScope); + } jyu2 wrote: > efriedma wrote: > > Do we need to make the same change

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

efriedma added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:4579 + if (CE->hasAPValueResult()) +mangleValueInTemplateArg(ParamType, CE->getResultAsAPValue(), false, + /*NeedExactType=*/true); I'm not su

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

efriedma added a comment. It's not unprecedented to add flags to copy the behavior of other compilers, to make porting easier, especially when it doesn't place much burden on compiler maintainers. But what compiler preserves the names/values of static variables by default? It's not the sort o

<    11   12   13   14   15   16   17   18   >