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

efriedma added a comment. Given the complexity here, I agree this is probably the best we can reasonably do. Code and test changes LGTM. That said, this is missing a release note. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86310/new/

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

efriedma added a comment. > For assembly, I'm not really comfortable with the fix you're proposing; it > relies on the order in which functions are defined in assembly. I think LLVM > usually ends up emitting module-level inline assembly before generated code, > but it seems fragile to depend

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

efriedma updated this revision to Diff 557651. efriedma added a comment. Fix the calling convention for functions returning C++ classes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157547/new/ https://reviews.llvm.org/D157547 Files:

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

efriedma added a comment. For assembly, I'm not really comfortable with the fix you're proposing; it relies on the order in which functions are defined in assembly. I think LLVM usually ends up emitting module-level inline assembly before generated code, but it seems fragile to depend on that

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

efriedma updated this revision to Diff 557592. efriedma added a comment. Updated with feedback from @dpaoliello and additional internal testing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157547/new/ https://reviews.llvm.org/D157547 Files:

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

efriedma updated this revision to Diff 557412. efriedma added a comment. Rebased Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157547/new/ https://reviews.llvm.org/D157547 Files: clang/lib/CodeGen/CGCXX.cpp llvm/include/llvm/IR/CallingConv.h

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

efriedma updated this revision to Diff 557335. efriedma added a comment. Revised so guest exit thunks actually work. Thanks for the tip about the symbols; I forgot to look at the object file in addition to the generated assembly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

efriedma updated this revision to Diff 557315. efriedma added a comment. Updated with my attempt at guest exit thunks. Seems to be working for simple cases, but I'm not sure this is actually working properly (I'm still seeing LNK1000 crashes). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D127221: [Clang] Enable -print-pipeline-passes in clang.

efriedma added a comment. I don't really like `extern cl::opt PrintPipelinePasses;` (as opposed to implementing a proper driver option that calls a proper API); secret handshakes like this make it harder for people to write non-clang frontends. But I won't block the patch just for that, I

[PATCH] D156172: [clang][CodeGen] Emit annotations for function declarations.

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

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

efriedma added a comment. Maybe split the changes to reformat the unittests into a separate patch? Otherwise, I'm happy with the current state of this patch, but probably someone more active in flang should approve. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D158857: [clang][aarch64] Add support for the MS qualifiers __ptr32, __ptr64, __sptr, __uptr for aarch64

efriedma added a comment. If you don't have autoupgrade, that means you can't link IR built with older versions of LLVM to IR built with newer versions. llvm::UpgradeDataLayoutString is designed to fix this incompatibility. It's probably worth doing here given that it's relatively

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

efriedma updated this revision to Diff 552917. efriedma added a comment. Update to address issues found so far. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157547/new/ https://reviews.llvm.org/D157547 Files: clang/lib/CodeGen/CGCXX.cpp

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

efriedma added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64CallingConvention.td:555-557 +def CSR_Win_AArch64_Arm64EC_Thunk : CalleeSavedRegs<(add X19, X20, X21, X22, X23, X24, + X25, X26, X27, X28, FP, LR,

[PATCH] D158267: [clang][CodeGen] Avoid emitting unnecessary memcpy of records without content

efriedma added a comment. This looks sort of similar to CodeGen::isEmptyRecord... but I guess it's not quite the same thing. (Even if a struct isn't "empty" in the ABI sense, there still might not be anything to copy.) Instead of looking for zero-length bitfields, you probably want to just

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

efriedma added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64MCInstLower.cpp:51-54 +// For ARM64EC, symbol lookup in the MSVC linker has limited awareness +// of ARM64EC mangling ("#"/"$$h"). So object files need to refer to both +// the mangled and

[PATCH] D156891: [CodeGen] Remove Constant arguments from linkage functions, NFCI.

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

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

efriedma added a comment. The test changes look good to me. Comment at: flang/lib/Decimal/CMakeLists.txt:52 -add_flang_library(FortranDecimal INSTALL_WITH_TOOLCHAIN - binary-to-decimal.cpp - decimal-to-binary.cpp -) +add_compile_options(-fPIC) + This

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

efriedma added a comment. I was probably thinking of that, yes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126864/new/ https://reviews.llvm.org/D126864 ___ cfe-commits mailing list

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

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

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

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

[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin

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

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

efriedma added a comment. RecordDecl::hasFlexibleArrayMember() is supposed to reflect the standard's definition of a flexible array member, which only includes incomplete arrays. The places that care about other array members use separate checks. We wouldn't want to accidentally extend the

[PATCH] D157757: [Headers] Replace __need_STDDEF_H_misc with specific __need_ macros

efriedma added inline comments. Comment at: clang/test/Headers/stddef.c:20-23 +// rsize_t will only be defined if __STDC_WANT_LIB_EXT1__ is set to >= 1. +// It would be nice to test the default undefined behavior, but that emits +// a note coming from stddef.h "rsize_t, did you

[PATCH] D157757: [Headers] Replace __need_STDDEF_H_misc with specific __need_ macros

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

[PATCH] D157757: [Headers] Replace __need_STDDEF_H_misc with specific __need_ macros

efriedma added inline comments. Comment at: clang/lib/Headers/stddef.h:112 #if defined(_MSC_EXTENSIONS) && defined(_NATIVE_NULLPTR_SUPPORTED) -namespace std { typedef decltype(nullptr) nullptr_t; } -using ::std::nullptr_t; +// __need_NULL would previously define nullptr_t for

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

efriedma added a comment. This approach looks fine. Comment at: clang/lib/CodeGen/CGCall.cpp:5781 // If the value is offset in memory, apply the offset now. + if (!isEmptyRecord(getContext(), RetTy, true)) { The isEmptyRecord call could use a

[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2697 +bool SanitizeBuiltin = SanOpts.has(SanitizerKind::Builtin); +bool SanitizeOverflow = SanOpts.has(SanitizerKind::SignedIntegerOverflow); + artem wrote: > efriedma wrote: > >

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

efriedma added a comment. You can't, in general, check whether a type is stored in a no_unique_address field. Consider the following; the bad store is in the constructor S2, but the relevant no_unique_address attribute doesn't even show up until the definition of S3. struct S {}; S f();

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

efriedma added a comment. Sure, diverging from MSVC here seems fine. I agree it's unlikely anyone would actually want to put a variable that will be modified in a "const" section. Comment at: clang/lib/Sema/SemaDecl.cpp:14254 int SectionFlags = ASTContext::PSF_Read; -

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

efriedma added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:14254 int SectionFlags = ASTContext::PSF_Read; -if (var->getType().isConstQualified()) { - if (HasConstInit) dblaikie wrote: > rnk wrote: > > rsmith wrote: > > > efriedma

[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

efriedma created this revision. efriedma added reviewers: dpaoliello, mstorsjo, bcl5980, jacek. Herald added subscribers: zzheng, hiraditya, kristof.beyls. Herald added a project: All. efriedma requested review of this revision. Herald added projects: clang, LLVM. Herald added a subscriber:

[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin

efriedma added a comment. The overall approach here seems reasonable. I mean, technically the undefined behavior is happening in the library, but detecting it early seems like a good idea. This approach does have a significant limitation, though: CGBuiltin won't detect cases that involve

[PATCH] D155850: [Clang][CodeGen][RFC] Add codegen support for C++ Parallel Algorithm Offload

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM (but please don't merge until we reach consensus on the overall feature) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155850/new/ https://reviews.llvm.org/D155850

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

efriedma added a comment. I see what you're getting at here... but I don't think this works quite right. If the empty class has a non-trivial constructor, we have to pass the correct "this" address to that constructor. Usually a constructor for an empty class won't do anything with that

[PATCH] D156466: [clang][CGExprConstant] handle implicit widening/narrowing Int-to-Int casts

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

[PATCH] D156378: [clang][CGExprConstant] handle unary negation on integrals

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

[PATCH] D156172: [clang][CodeGen] Emit annotations for function declarations.

efriedma added a comment. I'm happy with this approach. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4327 +if (D && D->hasAttr() && isa(Entry)) + DeferredAnnotations[cast(Entry)] = cast(D); + This doesn't quite seem sufficient... I'd expect you'd

[PATCH] D155850: [Clang][CodeGen][RFC] Add codegen support for C++ Parallel Algorithm Offload

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:5559 +return EmitStdParUnsupportedBuiltin(this, FD); + else +ErrorUnsupported(E, "builtin function"); Else-after-return. Comment at:

[PATCH] D156482: [clang][CGExprConstant] handle FunctionToPointerDecay

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1128 +case CK_NoOp: +case CK_NonAtomicToAtomic: return Visit(subExpr, destType); I think I'd prefer to continue treating an undecayed function as an "lvalue", to

[PATCH] D156466: [clang][CGExprConstant] handle implicit widening/narrowing Int-to-Int casts

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1152 + return CI; +llvm::APInt A = CI->getValue().sextOrTrunc(DstWidth); +return llvm::ConstantInt::get(CGM.getLLVMContext(), A); sextOrTrunc()

[PATCH] D156378: [clang][CGExprConstant] handle unary negation on integrals

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1366 + llvm::Constant *VisitUnaryOperator(UnaryOperator *U, QualType T) { +switch (U->getOpcode()) { StmtVisitor supports defining a function "VisitUnaryMinus", so you don't

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. In D76096#4540442 , @nickdesaulniers wrote: > Consider the following code: > > long x [] = {1, 2, 3, 4, 5 + 4}; > > Even with some of my recent

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

efriedma added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:14254 int SectionFlags = ASTContext::PSF_Read; -if (var->getType().isConstQualified()) { - if (HasConstInit) rnk wrote: > I think this is not compatible with MSVC. MSVC uses

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. That's scary: it means we can silently behave differently from other compilers in a way that's hard to understand. Is there some way we can provide a diagnostic? That said, it looks

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

efriedma added a comment. The general approach seems fine. The multiplier for constexpr vs. constant folding can be left for a followup, and we can continue to consider other possible improvements elsewhere. I guess I have one remaining question here: how does this interact with SFINAE? In

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

efriedma added a comment. > But prior to D151587 , we did that for C++. > Why is C special here? And prior to this patch, we did that for C++ 11+. Why > is C++ 03 special here? I'm trying to avoid regressions. C++11 made constant evaluation a lot more

[PATCH] D156175: [clang][ConstExprEmitter] handle NullToPointer ImplicitCastExpr

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

[PATCH] D156274: [NFC] Avoid potential dereferencing of nullptr.

efriedma added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:1659 mangleCXXDtorType(Dtor_Complete); +assert(ND); writeAbiTags(ND, AdditionalAbiTags); aaron.ballman wrote: > This seems incorrect -- if the declaration name is a

[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"); + } tnfchris wrote: > efriedma wrote: > > Why 1024? >

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

efriedma added inline comments. Comment at: flang/runtime/CMakeLists.txt:251 - INSTALL_WITH_TOOLCHAIN -) +if (DEFINED LLVM_ENABLE_RUNTIMES AND "flang-rt" IN_LIST LLVM_ENABLE_RUNTIMES) + add_flang_library(FortranRuntime STATIC klausler wrote: > pscoro wrote:

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

efriedma added a comment. Basically, I don't want order-of-magnitude compile-time regressions with large global variables. There are basically two components to that: - That the fast path for emitting globals in C continues be fast. - That we rarely end up using evaluateValue() on large global

[PATCH] D156185: [clang][ConstExprEmitter] handle ArrayToPointerDecay ImplicitCastExpr of StringLiterals

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. I don't have any concern with this specific patch; LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156185/new/

[PATCH] D156175: [clang][ConstExprEmitter] handle NullToPointer ImplicitCastExpr

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1131-1132 +case CK_NullToPointer: { + if (llvm::Constant *C = Visit(subExpr, destType)) +if (C->isNullValue()) + return CGM.EmitNullConstant(destType);

[PATCH] D156172: [clang][CodeGen] Emit annotations for function declarations.

efriedma added a comment. Slightly messed up my example because I forgot the function was unprototyped. The following should show what I mean: void foo(void); void *xxx = (void*)foo; __attribute__((annotate("bar"))) void foo(){} In terms of where the right place is, I don't recall the

[PATCH] D156185: [clang][ConstExprEmitter] handle ArrayToPointerDecay ImplicitCastExpr of StringLiterals

efriedma added a comment. No, that's taking the address of a string literal lvalue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156185/new/ https://reviews.llvm.org/D156185 ___ cfe-commits mailing

[PATCH] D156172: [clang][CodeGen] Emit annotations for function declarations.

efriedma added a comment. The idea of emitting annotations on declarations seems fine. (LLVM itself doesn't consume annotations anyway; they're meant as an extension mechanism for third-party tools.) I'm a bit concerned the way this is implemented will end up dropping annotations we would

[PATCH] D156185: [clang][ConstExprEmitter] handle ArrayToPointerDecay ImplicitCastExpr of StringLiterals

efriedma added a comment. > so if you were concerned with arrays of string literals, we need this patch. Ohhh that's not what I meant by StringLiterals. I meant cases where the string literal is an rvalue, like `char foo[10] = "x";`. If it's an lvalue, we go through

[PATCH] D156185: [clang][ConstExprEmitter] handle ArrayToPointerDecay ImplicitCastExpr of StringLiterals

efriedma added a comment. > I guess I lost focus on the "struct or array part." Right... scalars weren't the concern for D76096 . They have much less overhead going through ExprConstant, and any evaluation of scalars isn't a regression because we currently

[PATCH] D156175: [clang][ConstExprEmitter] handle NullToPointer ImplicitCastExpr

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1131-1132 +case CK_NullToPointer: { + if (llvm::Constant *C = Visit(subExpr, destType)) +if (C->isNullValue()) + return CGM.EmitNullConstant(destType);

[PATCH] D156185: [clang][ConstExprEmitter] handle ArrayToPointerDecay ImplicitCastExpr of StringLiterals

efriedma added a comment. We can do a whole bunch of these, but at some point we hit diminishing returns... bailing out here isn't *that* expensive. Do you have some idea how far you want to go with this? Adding a couple obvious checks like this isn't a big deal, but I don't want to build up

[PATCH] D156182: [clang][CodeGenModule] remove declaration of GetAddrOfConstantString

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

[PATCH] D156154: [clang][ConstExprEmitter] handle IntegerLiterals

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. That said, skipping all those abstraction layers for the simplest cases is probably worth it; LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D156154: [clang][ConstExprEmitter] handle IntegerLiterals

efriedma added a comment. We don't completely fall back if a subexpression fails to evaluate. EmitArrayInitialization etc. don't recursively visit; they use tryEmitPrivateForMemory, so we can fallback for subexpressions. (tryEmitPrivateForMemory can still fail for certain constructs, like

[PATCH] D156154: [clang][ConstExprEmitter] handle IntegerLiterals and IntegralCasts

efriedma added a comment. As a practical matter, I'm not sure this helps much; ExprConstant should be reasonably fast for simple integers. The performance issues only really show for structs/arrays. But maybe it helps a little to handle a few of the most common integer expressions.

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

efriedma added a comment. > Note sure what you mean. Making sure we use size_t for all array extents is > not something that can be fixed overnight but more importantly, it does not > help: Would it not overflow, the allocation would still fail. Oh, right, it would be sizeof(APValue) *

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

efriedma added a comment. The UINT_MAX thing seems like a straightforward bug; if we have time to fix it properly, I'd prefer not to add weird workarounds. The release note says "unless they are part of a constant expression", but I don't see any code in the implementation that distinguishes

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

efriedma added a comment. > Is the idea for the way forward here to ensure (i.e. adding code such) that > ConstExprEmitter can constant evaluate such Expr's? For that exact construct, EvaluateAsRValue will also fail, so there's no real regression. The issue would be for a constant global

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

efriedma added a comment. Hmm, that kind of construct could run into issues with large global variables. If we "inline" a bunch of large global constants into initializers for arrays, we could significantly increase codesize. Not sure how likely that is in practice. We could maybe consider

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

efriedma added a comment. The ones most likely to be a concern are InitListExpr and StringLiteral. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76096/new/ https://reviews.llvm.org/D76096 ___

[PATCH] D76096: [clang] allow const structs/unions/arrays to be constant expressions for C

efriedma added a comment. The idea would be looking for places we EvaluateAsRValue an array or struct. Not sure what that stack trace represents. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76096/new/ https://reviews.llvm.org/D76096

[PATCH] D76096: [clang] allow const structs to be constant expressions for C

efriedma added a comment. My primary concern here is making sure we don't actually blow up compile-time. D151587 fixes the dependency from CGExprConstant, which was the most complicated one, but there are other places that call into Evaluate(). Some of

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

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

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

efriedma added inline comments. Comment at: clang/lib/AST/Expr.cpp:3444 if (CE->getCastKind() == CK_NoOp || CE->getCastKind() == CK_LValueToRValue || CE->getCastKind() == CK_ToUnion || nickdesaulniers wrote: > efriedma wrote: > > efriedma

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

efriedma added a comment. > restore E->getStorageDuration() == SD_Static check to fix libcxx regressions Makes sense... but it would be nice to add a testcase for whatever construct was triggering this. Comment at: clang/lib/AST/Expr.cpp:3462-3468

[PATCH] D155850: [Clang][CodeGen][RFC] Add codegen support for C++ Parallel Algorithm Offload

efriedma added a comment. > In what regards how to do deferred diagnostics, it think it can be done like > this (I crossed streams in my prior reply when discussing this part, so it's > actually nonsense): instead of emitting undef here, we can emit a builtin > with the same signature, but

[PATCH] D155850: [Clang][CodeGen][RFC] Add codegen support for C++ Parallel Algorithm Offload

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:5542 + if (!getLangOpts().HIPStdPar) +ErrorUnsupported(E, "builtin function"); AlexVlx wrote: > efriedma wrote: > > This doesn't make sense; we can't just ignore bits of the

[PATCH] D76096: [clang] allow const structs to be constant expressions for C

efriedma added inline comments. Comment at: clang/test/Sema/builtins.c:181-186 + ASSERT(!OPT(test17_c)); + ASSERT(!OPT(_c[0])); + ASSERT(!OPT((char*)test17_c)); ASSERT(!OPT(test17_d));// expected-warning {{folding}} ASSERT(!OPT(_d[0]));// expected-warning

[PATCH] D155850: [Clang][CodeGen][RFC] Add codegen support for C++ Parallel Algorithm Offload

efriedma requested changes to this revision. efriedma added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:5542 + if (!getLangOpts().HIPStdPar) +ErrorUnsupported(E, "builtin function"); This

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

efriedma added inline comments. Comment at: clang/lib/AST/Expr.cpp:3462-3468 ->isConstantInitializer(Ctx, false, Culprit); case CXXDefaultArgExprClass: return cast(this)->getExpr() ->isConstantInitializer(Ctx, false, Culprit); case

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

efriedma added a comment. This seems to be close to a good state. Comment at: clang/lib/AST/Expr.cpp:3444 if (CE->getCastKind() == CK_NoOp || CE->getCastKind() == CK_LValueToRValue || CE->getCastKind() == CK_ToUnion || An

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

efriedma added a comment. > Indeed, but I wonder why only do this when IsForRef == true (narrator: it's > not)? A MaterializeTemporaryExpr is an lvalue; it only makes sense in lvalue contexts. > Also, why for MaterializeTemporaryExpr manually force the ForRef parameter to > false? The

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

efriedma added a comment. This seems to work: diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp index 25d3535e..98b1e4d 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -3319,6 +3319,10 @@ bool Expr::isConstantInitializer(ASTContext , bool IsForRef,

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

efriedma added inline comments. Comment at: flang/runtime/CMakeLists.txt:251 - INSTALL_WITH_TOOLCHAIN -) +if (DEFINED LLVM_ENABLE_RUNTIMES AND "flang-rt" IN_LIST LLVM_ENABLE_RUNTIMES) + add_flang_library(FortranRuntime STATIC pscoro wrote: > efriedma wrote:

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:836 +fnPtr = +llvm::ConstantExpr::getAddrSpaceCast(fnPtr, CGM.GlobalsInt8PtrTy); + return builder.add(fnPtr); If I follow correctly, the fnPtr is guaranteed to

[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; efriedma wrote: > nickdesaulniers wrote: > > efriedma

[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; nickdesaulniers wrote: > efriedma wrote: > >

[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; nickdesaulniers wrote: > nickdesaulniers wrote: > >

[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; nickdesaulniers wrote: > efriedma wrote: > > Checking

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

efriedma added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658 +if (auto *G = dyn_cast(Jump)) { + for (AddrLabelExpr *L : G->labels()) { nickdesaulniers wrote: > efriedma wrote: > > nickdesaulniers wrote: > > > rjmccall wrote: > >

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

efriedma added inline comments. Comment at: flang/runtime/CMakeLists.txt:251 - INSTALL_WITH_TOOLCHAIN -) +if (DEFINED LLVM_ENABLE_RUNTIMES AND "flang-rt" IN_LIST LLVM_ENABLE_RUNTIMES) + add_flang_library(FortranRuntime STATIC pscoro wrote: > efriedma wrote:

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

efriedma added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658 +if (auto *G = dyn_cast(Jump)) { + for (AddrLabelExpr *L : G->labels()) { nickdesaulniers wrote: > rjmccall wrote: > > nickdesaulniers wrote: > > > rjmccall wrote: > >

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

efriedma added inline comments. Comment at: flang/lib/Decimal/CMakeLists.txt:54 + +if (DEFINED LLVM_ENABLE_PROJECTS AND "flang" IN_LIST LLVM_ENABLE_PROJECTS) + add_flang_library(FortranDecimal I think it would make sense to use explicit CMake variables for the

[PATCH] D155175: [Clang] Fix consteval propagation for aggregates and defaulted constructors

efriedma added a comment. Test changes look fine. (I'm not really comfortable reviewing the Sema logic.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155175/new/ https://reviews.llvm.org/D155175 ___

[PATCH] D155175: [Clang] Fix consteval propagation for aggregates and defaulted constructors

efriedma added a comment. > You have something specific in mind? I was thinking something along the lines of the original testcase in 63742, which successfully compiled in 16, started producing an assertion on main, and successfully compiles with this patch. Repository: rG LLVM Github

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

efriedma added a comment. I'm not personally involved with any workflows that care about autoupgrade, so I'm not really invested in ensuring it's stable. If everyone agrees the impact is small enough that we're willing to just break autoupgrade to the extent it's relevant, I'll withdraw my

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

efriedma added a comment. In D86310#4498721 , @hvdijk wrote: > In D86310#4498575 , @efriedma wrote: > >> https://reviews.llvm.org/D86310#2231136 has an example where IR generated by >> clang breaks. > > clang

[PATCH] D155175: [Clang] Fix consteval propagation for aggregates and defaulted constructors

efriedma added a comment. Do we need CodeGen testcases here for full coverage? The testcases from the issue passed with -fsyntax-only. With this patch, the following cases produce errors that don't really make sense to me: consteval int f(int x) { return x; } struct SS

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

efriedma added a comment. > The only problem that approach 2 solves is to ensure that a non-clang > frontend using i128 https://reviews.llvm.org/D86310#2231136 has an example where IR generated by clang breaks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86310/new/

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

efriedma added a comment. > @efriedma would you consider the changes suggested by @hvdijk sufficient > under any circumstances or would you still insist on fully compatible > AutoUpgrade, given the above discussion? If the requirement is "we can mix old and new IR", we have to do it correctly,

  1   2   3   4   5   6   7   8   9   10   >