[PATCH] D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible

efriedma added a comment. Under this scheme, is it illegal to link together object files built with -ffine-grained-bitfield-accesses and object files built with -fno-fine-grained-bitfield-accesses? Do we want to add a temporary option to control this, to make it easier for people to benchmark

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

efriedma added a comment. I suspect the refactoring in the latest version of the patch accidentally made UBSan a bit more strict by default. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126864/new/ https://reviews.llvm.org/D126864 ___ cfe-c

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

efriedma added a comment. Could you lay out the expected interaction between "STDC FENV_ACCESS", "clang fp exceptions", "float_control", and "fenv_access"? If there's some way to map everything to "#pragma clang fp", please lay that out; if that isn't possible, please explain why. As far as I

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

efriedma added a comment. For FENV_ROUND, I think we should try to stick to the standard as closely as possible in Sema; since the standard models FENV_ROUND as a separate state, Sema should also model it as a separate state. If CodeGen decides it needs to modify the FP environment to actually

[PATCH] D126494: [clang] avoid assert due to device treated long double as double

efriedma added a comment. This doesn't make sense to me. If clang and LLVM disagree about the layout of a type, you are going to run into trouble. If you haven't yet, it's a matter of time. Looking at AMDGPUTargetInfo::setAuxTarget, maybe LongDoubleFormat is somehow inconsistent with LongDou

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

efriedma added a comment. I mean that ActOnPragmaFEnvAccess shouldn't call hasRoundingModeOverride()/setRoundingModeOverride(), and setRoundingMode shouldn't call getAllowFEnvAccess(). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126364/new/ htt

[PATCH] D116088: [compiler-rt] Implement ARM atomic operations for architectures without SMP support

efriedma added a comment. ldr+op+swp still isn't atomic. For each point in the code, please try the exercise of "what if my code is interrupted here"? The only way to use swp to implement general atomic operations is to use a separate spinlock. Repository: rG LLVM Github Monorepo CHANGES

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

efriedma added inline comments. Comment at: llvm/lib/Target/ARM/ARMFrameLowering.cpp:732 + // AAPCS requires use of R11, and PACBTI gets in the way of regular pushes, + // so FP ends up on area two. if (HasFP) { I guess this is related to this patch because

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

efriedma added a comment. > setRoundingMode definitely should not call getAllowFEnvAccess() and it does > not Yes, it does? // C2x: 7.6.2p3 If the FE_DYNAMIC mode is specified and FENV_ACCESS is "off", // the translator may assume that the default rounding mode is in effect. if (FPR == l

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

efriedma added a comment. The way I see it, there are two possibilities here: 1. In Sema, we have two rounding modes that correspond to FE_DYNAMIC: llvm::RoundingMode::Dynamic, and llvm::RoundingMode::NearestTiesToEven, plus some boolean to indicate whether the user actually explicitly specifie

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

efriedma added a comment. > Constrained intrinsics don't change the rounding mode. The standard text for FENV_ROUND requires that when we see a floating point operation, we have to perform it using the specified rounding mode. "floating-point operators [...] shall be evaluated according to the

[PATCH] D126764: [clang] [ARM] Add __builtin_sponentry like on aarch64

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

[PATCH] D126816: Only issue warning for subtraction involving null pointers on live code paths

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

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

efriedma added a comment. Probably IsTailPaddedMemberArray() in SemaChecking.cpp and isFlexibleArrayMemberExpr() in CGExpr.cpp should also check for this flag. Not sure if there's anything else that does similar checks; the static analyzer has its own flag consider-single-element-arrays-as-fle

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

efriedma added a comment. Given we have getEffectiveRoundingMode(), I think the calls to setRoundingModeOverride shouldn't be necessary? And if we drop those calls, we can also drop the IsRoundingModeSet variable. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revi

[PATCH] D126871: [clang] [MinGW] Use SEH for unwind info on ARM by default

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

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

efriedma added a comment. Shouldn't the rounding mode be FE_DYNAMIC by default? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126364/new/ https://reviews.llvm.org/D126364 ___ cfe-commits mailing list cfe

[PATCH] D126871: [clang] [MinGW] Use SEH for unwind info on ARM by default

efriedma added a comment. Oh, just remembered, we should probably release-note this. (You can post that as a followup.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126871/new/ https://reviews.llvm.org/D126871 __

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

efriedma added a comment. In D126364#3560984 , @sepavloff wrote: > In D126364#3560877 , @efriedma > wrote: > >> Shouldn't the rounding mode be FE_DYNAMIC by default? > > According to the standard it must be FE_TO

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

efriedma added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:622 setFPContractMode(LangOptions::FPM_Off); setRoundingMode(static_cast(LangOptions::FPR_ToNearest)); setFPExceptionMode(LangOptions::FPE_Ignore); I'm suggesting t

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

efriedma added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:622 setFPContractMode(LangOptions::FPM_Off); setRoundingMode(static_cast(LangOptions::FPR_ToNearest)); setFPExceptionMode(LangOptions::FPE_Ignore); sepavloff wrote:

[PATCH] D126871: [clang] [MinGW] Use SEH for unwind info on ARM by default

efriedma added a comment. > Btw, thanks a lot for your reviews, you've spent a lot of time on them! You're welcome. > But for MSVC style (C++ exceptions and __try/__except) there's still a couple > backend things that need to be implemented. Do you happen to know roughly how > much effort that

[PATCH] D126871: [clang] [MinGW] Use SEH for unwind info on ARM by default

efriedma added a comment. Funclets are necessary for both C++ and SEH exception handling, yes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126871/new/ https://reviews.llvm.org/D126871 ___ cfe-commits m

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

efriedma added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:622 setFPContractMode(LangOptions::FPM_Off); setRoundingMode(static_cast(LangOptions::FPR_ToNearest)); setFPExceptionMode(LangOptions::FPE_Ignore); sepavloff wrote:

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

efriedma added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:622 setFPContractMode(LangOptions::FPM_Off); setRoundingMode(static_cast(LangOptions::FPR_ToNearest)); setFPExceptionMode(LangOptions::FPE_Ignore); sepavloff wrote:

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

efriedma added a comment. > As for SOCK_MAXADDRLEN, that's a horrid hack, and the definition of struct > sockaddr needs to change. :) Do we want some builtin define so headers can check if we're in -fstrict-flex-arrays mode? It might be hard to mess with the definitions otherwise. CHANGES S

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

efriedma added inline comments. Comment at: llvm/test/CodeGen/Thumb/frame-access.ll:335 +; CHECK-FP-AAPCS: mov r1, r11 +; CHECK-FP-AAPCS: ldr r0, [r0, r1] +; CHECK: bl i This sequence requires, in general, scavenging two registers. I'm not sure we can do that i

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

efriedma added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:622 setFPContractMode(LangOptions::FPM_Off); setRoundingMode(static_cast(LangOptions::FPR_ToNearest)); setFPExceptionMode(LangOptions::FPE_Ignore); sepavloff wrote:

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

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM The Thumb1 changes here are complicated, so I'm not sure we found all the issues, but you fixed all the issues I spotted. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

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

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

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

[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] 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/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: > efriedma wrote: > > nickdesaulniers wrote: > > > rjmccall 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 is

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

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

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

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

[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] 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 &Ctx, bool IsForRef,

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

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

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

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

[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(&test17_c[0])); + ASSERT(!OPT((char*)test17_c)); ASSERT(!OPT(test17_d));// expected-warning {{folding}} ASSERT(!OPT(&test17_d[0]));// expect

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

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

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

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

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

[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/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 ___ cfe-commits

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

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

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

[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) * UINT_MAX

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

[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 https://reviews.llvm

[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] 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] 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); nick

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

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

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

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

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

[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); nick

[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/ https://reviews.llvm.org/D

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

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

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

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

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

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

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

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

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

[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 cfe-commits@lists.l

[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] 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 cfe-commits@lists.l

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

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

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

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

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