[PATCH] D136203: [ARM] Support -mexecute-only with -mlong-calls.

efriedma added a comment. The construct you want is pretty similar to a GOT. if you compile with -fPIE -fsemantic-interposition, you get basically the code you want, except that the compiler uses a plt by default instead of a got. If we supported -fno-plt for ARM, it would be almost exactly wh

[PATCH] D136203: [ARM] Support -mexecute-only with -mlong-calls.

efriedma added a comment. One, -mlong-calls isn't currently compatible with PIE. Two, on ARM, there are no special plt relocations; the linker just takes care of it. (You can see the differences if you try to take the address of a function without calling it.) Repository: rG LLVM Github Mo

[PATCH] D136203: [ARM] Support -mexecute-only with -mlong-calls.

efriedma added a comment. > I am thinking about adding a new option, say -mgot-calls to allow code > generation with the extra indirection. Is it sensible and shall I create > another diff to discuss that? That probably makes sense, yes. Comment at: llvm/lib/Target/ARM/ARMIS

[PATCH] D136203: [ARM] Support -mexecute-only with -mlong-calls.

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM with one small change. Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:779 // Supported only on ARMv6T2 and ARMv7 and above. // Cannot be combined wit

[PATCH] D134928: [Sema] Don't treat a non-null template argument as if it were null.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG795f67934d38: [Sema] Don't treat a non-null template argument as if it were null. (authored by efriedma). Changed prior to commit: https://reviews

[PATCH] D136548: [clang][CodeGen] Consistently return nullptr Values for void builtins and scalar initalization

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/D136548/new/ https://reviews.llvm.org/D136548 ___ cfe-commits mailing list cfe-commi

[PATCH] D136203: [ARM] Support -mexecute-only with -mlong-calls.

This revision was automatically updated to reflect the committed changes. Closed by commit rG7e8af2fc0c06: [ARM] Support -mexecute-only with -mlong-calls. (authored by ZhiyaoMa98, committed by efriedma). Changed prior to commit: https://reviews.llvm.org/D136203?vs=469049&id=470236#toc Reposito

[PATCH] D126903: [clang] Add support for __builtin_memset_inline

efriedma added a comment. It's not really a regression, just a bug in the feature. The rpmalloc code is checking `__has_builtin(__builtin_memset_inline)`, so older versions of clang aren't seeing the same source code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D136776: [clang codegen] Fix __try/__finally blocks in C++ constructors.

efriedma created this revision. efriedma added reviewers: rjmccall, rnk, mstorsjo. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. We were crashing trying to construct a GlobalDecl from a CXXConstructorDecl in the mangler. Instead of trying

[PATCH] D134089: [clang] Mention vector in the description for -mno-implict-float.

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM We might want per-target documentation somewhere for what counts as an "floating point or vector instruction". It's not entirely obvious whether, for example, `cvtss2si (%rax), %ecx

[PATCH] D131194: [C++20] Fix crash-on-valid with consteval temporary construction through list initialization

efriedma accepted this revision as: efriedma. efriedma added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1402 +RetType = CGM.getContext().getLValueReferenceType(RetType); + assert(!RetType.isNull() && "N

[PATCH] D125418: [Arm64EC 6/?] Implement C/C++ mangling for Arm64EC function definitions.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5128 +// to the function itself; it points to a stub for the compiler. +// FIXME: We also need to emit an entry thunk. +SmallString<256> MangledName; bcl5980 wrote: > efri

[PATCH] D125418: [Arm64EC 6/?] Implement C/C++ mangling for Arm64EC function definitions.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5128 +// to the function itself; it points to a stub for the compiler. +// FIXME: We also need to emit an entry thunk. +SmallString<256> MangledName; bcl5980 wrote: > efri

[PATCH] D127233: [CodeGen] Sort llvm.global_ctors by lexical order before emission

efriedma added a comment. > is this an ABI breaking change It only affects the order of initialization within a file, so it doesn't really have any implications for the binary ABI. It might break code, of course, but that's a different issue. If we want a flag to maintain the old behavior, we

[PATCH] D131980: [Passes] Don't run tail-call-elim in -O1

efriedma added a comment. Are you concerned about tail-call marking, or the recursive call->loop transform? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131980/new/ https://reviews.llvm.org/D131980 ___

[PATCH] D131980: [Passes] Don't run tail-call-elim in -O1

efriedma added a comment. If you're specifically concerned about sibcall (call->jmp) optimization in the backend, it might be better to adjust the backend to avoid sibcalls at -O1, as opposed to messing with optimization passes. (i.e. make -fno-optimize-sibling-calls the default at -O1.) "tai

[PATCH] D127233: [CodeGen] Sort llvm.global_ctors by lexical order before emission

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:580 +I = DelayedCXXInitPosition.find(D); +unsigned LexOrder = I == DelayedCXXInitPosition.end() ? ~0U : I->second; +AddGlobalCtor(Fn, 65535, LexOrder, COMDATKey); ychen wrote:

[PATCH] D127233: [CodeGen] Sort llvm.global_ctors by lexing order before emission

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:580 +I = DelayedCXXInitPosition.find(D); +unsigned LexOrder = I == DelayedCXXInitPosition.end() ? ~0U : I->second; +AddGlobalCtor(Fn, 65535, LexOrder, COMDATKey); ychen wrote:

[PATCH] D127233: [CodeGen] Sort llvm.global_ctors by lexing order before emission

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:580 +I = DelayedCXXInitPosition.find(D); +unsigned LexOrder = I == DelayedCXXInitPosition.end() ? ~0U : I->second; +AddGlobalCtor(Fn, 65535, LexOrder, COMDATKey); ychen wrote:

[PATCH] D127233: [CodeGen] Sort llvm.global_ctors by lexing order before emission

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

[PATCH] D119296: KCFI sanitizer

efriedma added inline comments. Comment at: clang/docs/ControlFlowIntegrity.rst:319 +cross-DSO function address equality. These properties make KCFI easier to +adopt in low-level software. KCFI is limited to indirect call checking only, +and isn't compatible with executable-only

[PATCH] D132275: [clang] Reset some attributed calling lambda

efriedma added a comment. Given that we mark up lambdas with the requirement that the pointer is nonnull/noundef/dereferenceable, we can't fix that by just messing with the attributes. Among other things, we could use a definition from a different module. Either we relax the requirements glob

[PATCH] D132275: [clang] Reset some attributed calling lambda

efriedma added a comment. In D132275#3740802 , @vitalybuka wrote: > In D132275#3740479 , @efriedma > wrote: > >> Among other things, we could use a definition from a different module. > > Is this a thing, I assu

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

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM (We might want to consider messing with the rules for calling lambdas that don't capture anything, since there's no way to actually access it from the lambda body. But it's unlikely

[PATCH] D132531: [AArch64] Reserve more physical registers

efriedma added a comment. > A reserved physical register doesn't mean it can't be used by > compiler/linker, it just means it can't be used by register allocator, see > the comments in TargetRegisterInfo::getReservedRegs(). The target feature to "reserve" a register is meant to be "don't use th

[PATCH] D133634: [clang] Allow vector of BitInt

efriedma added a comment. I think the worst of the backend issues with vectors of non-byte-size integers have been resolved? Work on that has progressed slowly for a very long time. There have been a constant stream of issues. (See, for example, D129164 .)

[PATCH] D134136: [Clang] Support constexpr for builtin fmax, fmin, ilogb, logb, scalbn

efriedma added a comment. These patches are hard to review, yes; the reviewer has to go through and verify for each function that the substitutes you've written actually match the C library semantics. (It's very easy to make mistakes with that sort of thing.) On that front, I'm pretty sure the

[PATCH] D125418: [Arm64EC 6/?] Implement C/C++ mangling for Arm64EC function definitions.

efriedma added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125418/new/ https://reviews.llvm.org/D125418 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cg

[PATCH] D134362: [clang] Fix interaction between asm labels and inline builtins

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:5057 +if (FD->hasAttr()) + FDInlineName = FD->getAttr()->getLabel().str(); +else This is throwing away the ".inline" suffix? Is that intentional? CHANGES SINCE LAST ACTION

[PATCH] D125418: [Arm64EC 6/?] Implement C/C++ mangling for Arm64EC function definitions.

efriedma added a comment. > Sometimes we will emit the alias here but later the function will be inlined > or eliminated by DCE. If the alias is externally visible, it can't be eliminated; the compiler can't tell whether the symbol is referenced. If the alias isn't externally visible, it's de

[PATCH] D134407: [FPEnv] Remove inaccurate comments regarding signaling NaN for isless

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

[PATCH] D134410: [clang][CodeGen] Add noundef metadata to load instructions (preliminary)

efriedma added a comment. How much code does this break? How much does this help performance? (My instinct here is that the tradeoff is not worth it, but I'm willing to be convinced otherwise.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D13441

[PATCH] D133711: [Sema] Reject array element types whose sizes aren't a multiple of their alignments

efriedma added a comment. Please try to avoid uploading multiple independent patches into the same revision; it's confusing for anyone who comes looking for the review of the initial patch, and only finds the followup. That said, fix looks fine, sure. Repository: rG LLVM Github Monorepo CH

[PATCH] D133993: [HLSL] Remove global ctor/dtor variable for non-lib profile.

efriedma added a comment. Why are we using different mechanisms for global constructors in "libraries" vs. other code? If we need a mechanism in LLVM already, we might as well use it all the time? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133

[PATCH] D134788: [ARM64EC][clang-cl] Add /arm64EC flag

efriedma added a comment. Please add a testcase verifying the interaction between "/arm64EC" and explicitly specifying "--target". (If we end up ignoring the "/arm64EC" flag, we probably want a warning.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D134369: [Clang] Support constexpr builtin fmax

efriedma added reviewers: jcranmer-intel, john.brawn, fhahn, sepavloff. efriedma added a comment. I think __builtin_fmax can raise a floating-point exception; in that case, it wouldn't be constant, I think? Not sure how consistent we are about handling that sort of thing in constant evaluation

[PATCH] D134369: [Clang] Support constexpr builtin fmax

efriedma added a comment. > We shouldn't be claiming that except maybe if we're using -fp-model=strict Makes sense. > I'm wondering what needs to happen with that for regular floating-point > operations that may trigger exceptions In general, floating-point exceptions are affected by whether w

[PATCH] D134369: [Clang] Support constexpr builtin fmax

efriedma added inline comments. Comment at: clang/test/Sema/constant-builtins-fmax.cpp:35-39 +#define FMAX_TEST_BOTH_ZERO(T, FUNC) \ +static_assert(0.0 == FUNC(0.0, 0.0)); \ +static_assert(0.0 == FUNC(-0.0, 0.0)); \ +static_assert(0.0 == FUNC(0.0, -0.0)); \ +

[PATCH] D134928: [Sema] Don't treat a non-null template argument as if it were null.

efriedma created this revision. efriedma added reviewers: aaron.ballman, erichkeane. efriedma added a project: clang. Herald added a project: All. efriedma requested review of this revision. The way this code checks whether a pointer is null is wrong for other reasons; it doesn't actually check w

[PATCH] D134928: [Sema] Don't treat a non-null template argument as if it were null.

efriedma updated this revision to Diff 464363. efriedma added a comment. Add release note. Add test coverage for C++17, since it uses a different codepath. (The C++17 codepath appears to work correctly without any changes.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D134928: [Sema] Don't treat a non-null template argument as if it were null.

efriedma added inline comments. Comment at: clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp:31 IP<&tl> ip7; // expected-error{{non-type template argument of type 'int *' is not a constant expression}} +IP<(int*)1> ip8; // expected-error {{non-type template argument does

[PATCH] D134788: [ARM64EC][clang-cl] Add /arm64EC flag

efriedma added a comment. Missing testcase for the case where the warning is triggered? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134788/new/ https://reviews.llvm.org/D134788 ___ cfe-commits mailing

[PATCH] D134788: [ARM64EC][clang-cl] Add /arm64EC flag

efriedma added a comment. I think we want to test both that the warning isn't triggered when --target isn't specified, and that the warning is triggered when --target is specified. Comment at: clang/test/Driver/cl-options.c:787 +// ARM64EC: "-triple" "arm64ec-pc-windows-msvc19

[PATCH] D133993: [HLSL] Remove global ctor/dtor variable for non-lib profile.

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. It seems like it would be better to put the code in an LLVM IR transform pass, if we can. It separates the concerns more clearly, and it would make life easier for other compilers. (If yo

[PATCH] D134369: [Clang] Support constexpr builtin fmax

efriedma added a comment. You might need to explicitly specify -std=c++17 in the RUN line. (That bot has a different target triple, and I think with that triple the default C++ standard isn't C++17?) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D135486: [Clang] Use C++17 in constant-builtins-fmax.cpp test

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. Looks fine Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135486/new/ https://reviews.llvm.org/D135486 _

[PATCH] D135429: [HLSL] [DirectX backend] Move generateGlobalCtorDtorCalls into DirectX backend.

efriedma added a comment. You might actually want to run this twice, once early, and once in the backend. Early to get high-quality optimization, late in case optimizations don't run for some reason. (Not sure how that works if `T.getEnvironment() == Triple::EnvironmentType::Library`, though,

[PATCH] D135429: [HLSL] [DirectX backend] Move generateGlobalCtorDtorCalls into DirectX backend.

efriedma added a comment. > The pass will be always enabled in > registerPipelineEarlySimplificationEPCallback (Not controlled by optimization > level), is it possible that things registered in > registerPipelineEarlySimplificationEPCallback be skipped? I think we won't end up running it at -O

[PATCH] D135429: [HLSL] [DirectX backend] Move generateGlobalCtorDtorCalls into DirectX backend.

efriedma added a comment. In D135429#3844172 , @efriedma wrote: >> The pass will be always enabled in >> registerPipelineEarlySimplificationEPCallback (Not controlled by >> optimization level), is it possible that things registered in >> registerPipeli

[PATCH] D135628: [clang][codegen] Don't emit atomic loads for threadsafe init if they aren't inline

efriedma created this revision. efriedma added reviewers: rjmccall, nikic. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. Performing a load before calling __cxa_guard_acquire is supposed to be an optimization, but it isn't much of one if we

[PATCH] D135628: [clang][codegen] Don't emit atomic loads for threadsafe init if they aren't inline

efriedma updated this revision to Diff 466657. efriedma added a comment. Upload correct version of patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135628/new/ https://reviews.llvm.org/D135628 Files: clang/lib/CodeGen/ItaniumCXXABI.cpp clan

[PATCH] D135628: [clang][codegen] Don't emit atomic loads for threadsafe init if they aren't inline

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG1079662d2fff: [clang][codegen] Don't emit atomic loads for threadsafe init if they aren't… (authored by efriedma). Repository: rG LLVM Github Mono

[PATCH] D130131: [HLSL] CodeGen hlsl cbuffer/tbuffer.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:131 case Decl::RequiresExprBody: + case Decl::HLSLBuffer: // None of these decls require codegen support. I'm a little confused by this. If it's possible to declare an HLSLBuffer

[PATCH] D134928: [Sema] Don't treat a non-null template argument as if it were null.

efriedma added a comment. Any suggestions for what the error message should look like, if you aren't satisfied with the current messages? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134928/new/ https://reviews.llvm.org/D134928 _

[PATCH] D135142: Use TI.hasBuiltinAtomic() when setting ATOMIC_*_LOCK_FREE values. NFCI

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

[PATCH] D130131: [HLSL] CodeGen hlsl cbuffer/tbuffer.

efriedma accepted this revision. efriedma added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/CodeGen/CGHLSLRuntime.cpp:112 +// Replace. +GV->replaceAllUsesWith(GEP); +// Erase GV. python3kgae wrote: > efriedm

[PATCH] D135832: Do not append terminating NUL to the string with embedded GPU binary.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGCUDANV.cpp:95 llvm::ConstantInt::get(SizeTy, 0)}; -auto ConstStr = CGM.GetAddrOfConstantCString(Str, Name.c_str()); +auto ConstStr = CGM.GetAddrOfConstantCString(Str, Name.c_st

[PATCH] D134928: [Sema] Don't treat a non-null template argument as if it were null.

efriedma added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:6490-6491 // C++11 [temp.arg.nontype]p1: // - an address constant expression of type std::nullptr_t if (Arg->getType()->isNullPtrType()) aaron.ballman wrote: > It's worth not

[PATCH] D134928: [Sema] Don't treat a non-null template argument as if it were null.

efriedma updated this revision to Diff 467601. efriedma added a comment. Add dedicated error message. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134928/new/ https://reviews.llvm.org/D134928 Files: clang/docs/ReleaseNotes.rst clang/include/c

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4761 + return Opts.CPlusPlus && D->hasConstantInitialization() && + D->getType()->isRecordType(); +} Not sure what the `D->getType()->isRecordType()` check is doing here. =

[PATCH] D138511: [CodeGen][AArch64] Fix AArch64ABIInfo::EmitAAPCSVAArg crash with empty record type in variadic arg

efriedma added a comment. Not sure what this is waiting on; do you need someone to merge for you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138511/new/ https://reviews.llvm.org/D138511 ___ cfe-commit

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4852 + if (!CD->isTrivial() && !D->getTLSKind()) +NeedsGlobalCtor = true; +} zahiraam wrote: > efriedma wrote: > > I have no idea what this code is suppos

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4852 + if (!CD->isTrivial() && !D->getTLSKind()) +NeedsGlobalCtor = true; +} zahiraam wrote: > efriedma wrote: > > zahiraam wrote: > > > efriedma wrote: >

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:254 +def warn_for_global_ctor_for_dllimport : Warning< + "global constructor is generated for dllimport global var %0">, Missing code to emit this warning? ===

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5006 +if (NeedsGlobalCtor || NeedsGlobalDtor) + DelayedCXXInitPosition[D] = ~0U; + } else { zahiraam wrote: > efriedma wrote: > > zahiraam wrote: > > > Do you agree this sho

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1926 +DiagnosticsEngine &Diags = CGM.getContext().getDiagnostics(); +Diags.Report(diag::warn_for_global_ctor_for_dllimport) << D; +return nullptr;

[PATCH] D140059: [APSInt] Fix bug in APSInt mentioned in https://github.com/llvm/llvm-project/issues/59515

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:5386 + if (InitInt) +InitExpr = DBuilder.createConstantValueExpression(InitInt.value()); +} else if (Init.isFloat()) I think we actually want the existing behavior here

[PATCH] D137268: [clang][Headers] Do not define varargs macros for __need___va_list

efriedma added inline comments. Comment at: clang/lib/Headers/stdarg.h:21 + +#ifdef __STDARG_H Maybe the following is a little more readable? ``` #ifndef __STDARG_H #ifndef __GNUC_VA_LIST #define __GNUC_VA_LIST 1 typedef __builtin_va_list __gnuc_va_list; #end

[PATCH] D136998: Try to implement lambdas with inalloca parameters by inlining the call operator function.

efriedma added a comment. Should we try to use this codepath for variadic lambdas as well? Do we want to try to unify our cloning code? CodeGenFunction::GenerateVarArgsThunk has code doing something similar. (It's at least worth comparing to see if you're doing something significantly differ

[PATCH] D136998: Try to implement lambdas with inalloca parameters by inlining the call operator function.

efriedma added a comment. Might also be worth considering if we can avoid cloning here. It should be possible to emit the lambda body into a separate function with a calling convention of your choice, and make both the call operator and the static invoker call it. Repository: rG LLVM Githu

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

efriedma added a comment. I'm not quite sure I understand what's happening here. Does this actually avoid generating two copies of the function body if both the call operator and the conversion are used? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

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

efriedma created this revision. efriedma added reviewers: nikic, t.p.northover, john.brawn, joerg, tomhughes, alanphipps, aykevl. Herald added subscribers: s.egerton, simoncook, hiraditya, kristof.beyls. Herald added a project: All. efriedma requested review of this revision. Herald added subscrib

[PATCH] D136998: Try to implement lambdas with inalloca parameters by inlining the call operator function.

efriedma added a comment. In D136998#3926321 , @rnk wrote: > In D136998#3906874 , @efriedma > wrote: > >> Should we try to use this codepath for variadic lambdas as well? > > Yes! > >> Do we want to try to unify

[PATCH] D138078: [CodeGenPrepare] split critical indirect edges from callbr w/ outputs

efriedma added a comment. You can't put transforms that are necessary for correctness in CodeGenPrepare; it isn't enabled at -O0. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138078/new/ https://reviews.llvm.org/D138078 _

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added a comment. There are multiple notions of "constant" here: - Whether a value is invariant at runtime. - Whether a value is legal in a "constexpr" expression. - Whether we can emit a constant without emitting code that runs at startup to initialize it. The third is generally a supe

[PATCH] D136776: [clang codegen] Fix __try/__finally blocks in C++ constructors.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG0fcb26c5b648: [clang] Fix __try/__finally blocks in C++ constructors. (authored by efriedma). Repository: rG LLVM Github Monorepo CHANGES SINCE L

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

efriedma added a comment. So gcc has two different behaviors on ARM: - On linux, prefers `__sync` calls, and generates inline code for load/store. - On baremetal, gcc chooses what sort of atomic call to generate based on how the source code is written: if the user writes `__sync`, you get `__syn

[PATCH] D136497: [Clang] support for outputs along indirect edges of asm goto

efriedma added inline comments. Comment at: clang/docs/LanguageExtensions.rst:1584 +Outputs may be used along any branches from the ``asm goto`` whether the +branches are taken or not. Maybe put a note about earlier releases, so people don't get confused if the

[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; Should thi

[PATCH] D142459: [clang] Deprecate uses of GlobalObject::getAlignment

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGCUDANV.cpp:491 new llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false, - llvm::Align(Var->getAlignment()), I); + Var->getAlign().valueOrO

[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. I'm having a little trouble following what the meaning of an "Address" is with this patch. The `Pointer` member refers to an encoded value, and calling getPointer() emits IR to decode it? Could use a few comments to explain the API. Repository: rG LLVM Github Mon

[PATCH] D143386: Add function pointer alignment to DataLayout

efriedma added a comment. Almost all the specifications you're specifying are wrong. Very few targets should be using "i". Off the top of my head, 32-bit ARM, MIPS targets with MicroMIPS, and certain PowerPC targets should, but most common targets shouldn't. ("i" means that either the target

[PATCH] D143205: [clang] add __has_extension(gnu_asm_goto_with_outputs_full)

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

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

efriedma added a comment. Using the FPOptions from the beginning of the file doesn't seem much better than the FPOptions at the end of the file. Don't we want to use the FPOptions from the point of definition of the template? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D141451: [clang] report inlining decisions with -Wattribute-{warning|error}

efriedma added a comment. There are two different approaches we use for getting source locations for diagnostics coming out of the backend; either embedding clang SourceLocations into IR, or trying to translate DWARF locations. The advantage of using clang SourceLocations is that we never lose

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

efriedma added subscribers: nikic, efriedma. efriedma added reviewers: nikic, jdoerfert. efriedma added a comment. From an IR semantics standpoint, I'm not sure the `memory(none)` marking is right if the function throws an exception. LangRef doesn't explicitly exclude the possibility, but take

[PATCH] D141451: [clang] report inlining decisions with -Wattribute-{warning|error}

efriedma added a comment. > Yeah, my thoughts too. Though if we don't compile with -g, does that mean if > I use DILocation (or w/e metadata for "inlining occurred") that .debug_info > will get emitted, even if the user didn't ask for it? If not, I can probably > switch everything to use DILoca

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

efriedma added a comment. I'm not trying to argue what the `pure`/`const` attribute in C/C++ is supposed to mean. I agree with your interpretation of the gcc documentation/implementation. I'm saying that there's a mismatch between the gcc `pure`/`const` and the LLVM IR `memory(read)`/`memory(

[PATCH] D133574: [C2x] reject type definitions in offsetof

efriedma added a comment. Given the extension appears to have real-world usage, I don't see any reason to break that usage. All known implementations parse this the way the user expects, there isn't any chance the user meant to write something else, and there isn't any chance it will mean some

[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] D140059: [APSInt] Fix bug in APSInt mentioned in https://github.com/llvm/llvm-project/issues/59515

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

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:520 + getOpenMPRuntime().emitDeclareTargetVarDefinition(D, Addr, PerformInit)) +return; + I don't really like the copy-pasted bits here. Comment at: clang/lib/

[PATCH] D127684: [NFC] Use `https` instead of `http` in the urls

efriedma added a comment. A couple notes skimming the changes: 1. If you're going to update URLs, please verify the new URLs are actually valid. 2. Some of the "URLs" you're updating aren't actually URLs. For example, the `xmlns` attribute in SVG files must be the exact string written in the s

[PATCH] D127460: Rename GCCBuiltin into ClangBuiltin

efriedma added a comment. Why are you messing with test_demangle.pass.cpp? The demangler doesn't care what symbols we actually define in LLVM... it's just a bunch of hardcoded testcases. So it doesn't matter if it continues to refer to GCCBuiltin. Other changes look fine. Repository: rG L

[PATCH] D127460: Rename GCCBuiltin into ClangBuiltin

efriedma added a comment. No other comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127460/new/ https://reviews.llvm.org/D127460 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://list

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays for stricter handling of flexible arrays

efriedma added a comment. Maybe we should document that zero-size arrays aren't treated as flexible arrays, since that's what the zero-size array extension was originally designed for. Looks fine otherwise. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126864/new/ https://reviews.llv

[PATCH] D126364: Fix interaction of pragma FENV_ACCESS with other pragmas

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

[PATCH] D125094: [ARM][Thumb] Command-line option to ensure AAPCS compliant Frame Records

efriedma accepted this revision. efriedma added a comment. LGTM with one minor comment (feel free to ignore if you disagree) Comment at: llvm/lib/Target/ARM/Thumb1FrameLowering.cpp:958 +static void popRegsFromStack(MachineBasicBlock &MBB, + MachineBa

[PATCH] D128482: [clang codegen] Add dso_local/hidden/etc. markings to VTT declarations

efriedma created this revision. efriedma added reviewers: rjmccall, MaskRay. Herald added a subscriber: StephenFan. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. We were marking definitions, but not d

[PATCH] D128482: [clang codegen] Add dso_local/hidden/etc. markings to VTT declarations

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe11bf8de729a: [clang codegen] Add dso_local/hidden/etc. markings to VTT declarations (authored by efriedma). Repository: rG LLVM Github Monorepo

<    6   7   8   9   10   11   12   13   14   15   >