[PATCH] D154923: [CodeGen] Support bitcode input containing multiple modules

efriedma added a comment. If I follow correctly, this is basically undoing the splitting that was done by the command that produced the bitcode file? I guess that could be useful. But it requires either renaming your object files from the default ".o" to ".bc", or explicitly passing "-x ir"?

[PATCH] D154911: Enabling fstack_clash_protection for arm32 bit, thumb and thumb2 mode

efriedma added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3460 + } else if (EffectiveTriple.isArm() || EffectiveTriple.isThumb()) { +CmdArgs.push_back("-mstack-probe-size=1024"); + } Why 1024? Comment at:

[PATCH] D153835: [Sema] Clone VisibilityAttr for functions in template instantiations

efriedma added a comment. Can you write the complete rule we're trying to follow here? Like, if you have a free function template, prioritize attributes in the following order ..., if you have a member function, use the following order ..., etc. I know that's a significant amount of writing,

[PATCH] D154270: [clang][CodeGen] Fix global variables initialized with an inheriting constructor.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb4bae3fd8ede: [clang][CodeGen] Fix global variables initialized with an inheriting… (authored by efriedma). Repository: rG LLVM Github Monorepo

[PATCH] D154284: [C11] Correct global atomic pointer initialization from an integer constant

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. The testcase doesn't actually trigger the assertion... but I guess it still tests the output, so that's probably okay? Maybe add a case `static _Atomic(int *) glob_pointer_from_long =

[PATCH] D154270: [clang][CodeGen] Fix global variables initialized with an inheriting constructor.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:2459 + if (!isa(Arg.getAnyValue())) +Arg.getAnyValue()->setName(D.getName()); rsmith wrote: > Is it feasible to only set the name if the value doesn't already have a name, > or do

[PATCH] D154270: [clang][CodeGen] Fix global variables initialized with an inheriting constructor.

efriedma created this revision. efriedma added reviewers: rsmith, rnk, rjmccall, akhuang. Herald added a subscriber: kristof.beyls. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. For inheriting constructors which have to be emitted inline,

[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

[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

[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

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

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

[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] 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 ={{.*}}

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

[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

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

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

[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

[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

[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

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

efriedma updated this revision to Diff 532334. efriedma added a comment. Restrict to AArch64. Actually, it seems like something sort of similar happens with x86 vectorcall. But I haven't tried to test all the permutations of that, so don't modify the behavior for now. Repository: rG LLVM

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

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/D137872/new/ https://reviews.llvm.org/D137872

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

efriedma created this revision. efriedma added reviewers: rnk, hans, dblaikie, sigatrev, samtebbs, rjmccall. Herald added subscribers: mstorsjo, kristof.beyls. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. MSVC normally has a bunch of

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

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1470 +->getLambdaStaticInvoker()) && + !Fn->getName().contains("__impl")) { +// If emitting a lambda with static invoker on X86 Windows, change

[PATCH] D152275: Use memory region declaration intrinsic when generating code for array subscripts

efriedma added a comment. In D152275#4423845 , @simeon wrote: >> - User code might not actually obey the language rules; do we have any >> sanitizer that checks if user code trips over this? > > I believe AddressSanitizer >

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

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:917 + bool VisitArraySubscriptExpr(const ArraySubscriptExpr *A) { +return Visit(A->getBase()) || Visit(A->getIdx()); + } Should only need to visit base, not idx. Repository:

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

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:1472 +// the call operator body. +EmitLambdaStaticInvokeBody(cast(FD)); } else if (FD->isDefaulted() && isa(FD) && akhuang wrote: > efriedma wrote: > > Does this pass

[PATCH] D152396: [clang][doc] Rescue some deleted bits of the command-line reference.

This revision was automatically updated to reflect the committed changes. Closed by commit rGf51924d124bd: [clang docs] Rescue some deleted bits of the command-line reference. (authored by efriedma). Changed prior to commit: https://reviews.llvm.org/D152396?vs=529422=531448#toc Repository:

[PATCH] D152275: Use memory region declaration intrinsic when generating code for array subscripts

efriedma added reviewers: aaron.ballman, courbet, nikic, jdoerfert, nlopes, jeroen.dobbelaere. efriedma added a comment. If we are going to do this at all, I think this is roughly what it should look like. Potential issues you might run into: - The compile-time overhead of creating a bunch of

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

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGClass.cpp:3097 + FD->getLocation(), FD->getLocation()); +CGF.EmitFunctionBody(FD->getBody()); +CGF.FinishFunction(); Is there any way we can use GenerateCode as the

[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: > > efriedma wrote: > > > This needs a comment explaining why we're bailing out here. > >

[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; + efriedma wrote: > This needs a comment explaining why we're bailing out here. We might need to do a recursive visit still, to

[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; + This needs a comment explaining why we're bailing out here. Comment at:

[PATCH] D132275: [clang] Create alloca to pass into static lambda

efriedma added a comment. > The first argument in the call is an undef but the argument type is also > marked as noundef, so this is unreachable It looks like your code was getting miscompiled? If I'm understanding correctly, without this patch, we assume the lambda body is unreachable, and

[PATCH] D152396: [clang][doc] Rescue some deleted bits of the command-line reference.

efriedma created this revision. efriedma added reviewers: serge-sans-paille, aaron.ballman. Herald added a subscriber: dschuff. Herald added a project: All. efriedma requested review of this revision. Herald added a subscriber: aheejin. Herald added a project: clang. Back when the command-line

[PATCH] D73459: [ARM] Add documentation for -march= and -mfpu= command line options

efriedma reopened this revision. efriedma added a comment. This revision is now accepted and ready to land. Herald added a project: All. This patch was reverted by 9624beb38a46037f69362650a52e06d8be4fd006 . I have a patch

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

efriedma added a comment. > Yeah, but not because of this patch; that's a pre-existing issue. Right; the _Atomic crashes are a pre-existing issue unrelated to MaterializeTemporaryExpr, so you shouldn't be trying to solve it by messing with HasAnyMaterializeTemporaryExpr. Repository: rG

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

efriedma added a comment. The following also crashes, with no MaterializeTemporaryExpr involved. struct X { short n; char c; }; struct Y { _Atomic(X) a; _Atomic(int) b; }; constexpr X x{}; int z; Y y = { x, z }; Repository: rG LLVM Github Monorepo CHANGES

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

efriedma added a comment. Two points I'm not sure about in the current version: - Handling MaterializeTemporaryExpr in ConstExprEmitter doesn't make sense; ConstExprEmitter is not supposed to visit lvalues. (And I'm not sure what the new check is supposed to do; `E->isGLValue()` is always true

[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() ? +

[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() ? +

[PATCH] D148094: [clang][CodeGen] Break up TargetInfo.cpp [8/8]

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/D148094/new/ https://reviews.llvm.org/D148094

[PATCH] D150215: [clang][CodeGen] Break up TargetInfo.cpp [7/8]

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/D150215/new/ https://reviews.llvm.org/D150215

[PATCH] D132275: [clang] Create alloca to pass into static lambda

efriedma added a comment. > EmitLambdaDelegatingInvokeBody emit an always_inline hint To clarify, I mean on the call instruction, not on either function. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132275/new/ https://reviews.llvm.org/D132275

[PATCH] D132275: [clang] Create alloca to pass into static lambda

efriedma added a comment. Possibly the alloca doesn't get eliminated, or doesn't get eliminated early enough for attributor to kick in. Not sure what context would make that significant, off the top of my head. Is the lambda in question defined in an inline function (linkonce_odr)? Maybe we

[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() ? +

[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

[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

[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() ? +

[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() ? +

[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() ? +

[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() ? +

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

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

[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

[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

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

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

[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

[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

[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

[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

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

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

[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] 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. I guess to make that work, the constant evaluator would need to track whether we're in an SFINAE context (Sema::isSFINAEContext()). Based on that, we'd need to explicitly generate an error if we're in an SFINAE context, and a warning if we're not in such a context.

[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. In D150226#4354063 , @efriedma wrote: >> Also note, one of the bugs I reference shows how this breaks SFINAE: >> https://github.com/llvm/llvm-project/issues/57176 and that is not easily >> fixable. So this is non-conforming

[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. > Also note, one of the bugs I reference shows how this breaks SFINAE: > https://github.com/llvm/llvm-project/issues/57176 and that is not easily > fixable. So this is non-conforming since it breaks valid code. You can mark a warning diagnostic SFINAEFailure to ensure

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

efriedma added a comment. At first glance, this seems like the wrong place to put this cast. If an expression in the AST produces a pointer with generic pointer type, then CodeGen should generate code for that expression that returns a generic pointer type. We shouldn't wait until the

[PATCH] D148723: [clang] Enforce external linkage for inline builtin original declaration

efriedma added a comment. Is there some reason we actually need to do this whole dance in C++? The whole point of "inline builtins" was to handle constructs in the glibc headers that involve implementations of libc functions that somehow end up recursively calling themselves instead of a real

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

efriedma added a comment. > This is an adaptation of the IBM XL compiler's -qstatsym option, which is > meant to generate symbol table entries for static variables. An artifact of > that compiler is that static variables are often not discarded even when > unused. Oh, I see; the compiler

[PATCH] D124642: [WIP] Add support for return from an SEH __finally block.

efriedma updated this revision to Diff 522782. efriedma added a comment. Fixup a couple LLVM tests which are failing; I think they're affected by the WinException.cpp changes? Maybe need to look a bit more closely to see if the changes make sense. Repository: rG LLVM Github Monorepo

[PATCH] D124642: [WIP] Add support for return from an SEH __finally block.

efriedma updated this revision to Diff 522773. efriedma added a comment. Herald added a subscriber: arichardson. Rebased so it builds, and added a couple tests, to unblock anyone wanting to look at this further. Haven't re-done my runtime testing. Still haven't addressed all the review

[PATCH] D124642: [WIP] Add support for return from an SEH __finally block.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGException.cpp:1833 +/// Find all local variable captures in the statement. +struct ReturnStmtFinder : ConstStmtVisitor { + bool ContainsRetStmt = false; rnk wrote: > We have the option to

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

efriedma added a comment. > I'd really want to look at IR for return inside try/finally, but for some > reason, I an not build compiler with your patch: no member named > 'setHasAddressTaken' in 'llvm::MachineBasicBlock'; did you mean > 'hasAddressTaken'. I may missing some thing in my

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

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM... but I don't think the IR we're generating is really correct overall; see https://github.com/llvm/llvm-project/issues/62723 On a side-note, other open issues related to -EHa/__try:

[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

[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

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

[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

[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

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

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

efriedma added a comment. Is there some alternative way we can write this? Even if each of the overrides is only technically used in one place, it's a repeating pattern, and writing the casts inline makes it really hard to read. (Maybe the helpers can be somewhere else?) Repository: rG

[PATCH] D148093: [clang][CodeGen] Break up TargetInfo.cpp [5/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/D148093/new/ https://reviews.llvm.org/D148093

<    1   2   3   4   5   6   7   8   9   10   >