[PATCH] D132843: [RISCV] Ensure target features get passed to the LTO linker for RISC-V

efriedma added a comment. In D132843#3771029 , @jrtc27 wrote: > In D132843#3770936 , @efriedma > wrote: > >>> There is also an issue with how to store and determine the final LTO target >>> features. For example

[PATCH] D132952: [Sema] disable -Wvla for function array parameters

efriedma added inline comments. Comment at: clang/test/Sema/warn-vla.c:8-12 +void test2(int n, int v[n]) { // c99 no-warning +#if __STDC_VERSION__ < 199901L +// expected-warning@-2{{variable length arrays are a C99 feature}} +#endif } aaron.ballman wrote: > aaro

[PATCH] D132952: [Sema] disable -Wvla for function array parameters

efriedma added inline comments. Comment at: clang/test/Sema/warn-vla.c:8-12 +void test2(int n, int v[n]) { // c99 no-warning +#if __STDC_VERSION__ < 199901L +// expected-warning@-2{{variable length arrays are a C99 feature}} +#endif } aaron.ballman wrote: > efri

[PATCH] D132952: [Sema] disable -Wvla for function array parameters

efriedma added inline comments. Comment at: clang/test/Sema/warn-vla.c:8-12 +void test2(int n, int v[n]) { // c99 no-warning +#if __STDC_VERSION__ < 199901L +// expected-warning@-2{{variable length arrays are a C99 feature}} +#endif } aaron.ballman wrote: > efri

[PATCH] D132952: [Sema] disable -Wvla for function array parameters

efriedma added inline comments. Comment at: clang/test/Sema/warn-vla.c:8-12 +void test2(int n, int v[n]) { // c99 no-warning +#if __STDC_VERSION__ < 199901L +// expected-warning@-2{{variable length arrays are a C99 feature}} +#endif } aaron.ballman wrote: > efri

[PATCH] D125419: [Arm64EC 7/?] clang side of Arm64EC varargs ABI.

efriedma updated this revision to Diff 458290. efriedma added a comment. Herald added subscribers: llvm-commits, arichardson. Herald added a project: LLVM. Update to handle va_arg Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125419/new/ https://re

[PATCH] D133570: Clang codegen, fixes issue with emitting partially initialized constant arrays larger than 2^32

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1225 assert(CAT && "can't emit array init for non-constant-bound array"); -unsigned NumInitElements = ILE->getNumInits(); -unsigned NumElements = CAT->getSize().getZExtValue(); +uint

[PATCH] D133570: Clang codegen, fixes issue with emitting partially initialized constant arrays larger than 2^32

efriedma added a comment. FYI, the "Visit" method comes from the base class StmtVisitor; it just forwards to the relevant method. (For example, if you call "Visit" with an InitListExpr, it calls VisitInitListExpr.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

[PATCH] D132952: [Sema] disable -Wvla for function array parameters

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

[PATCH] D132916: [clang] Explicitly set the EmulatedTLS codegen option

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM (I think you can call it "NFC" if there's no visible change in community; you don't have to try to account for downstream changes that might interact with it.) Repository: rG LLV

[PATCH] D133711: [Sema] Reject array element types whose alignments are larger than their sizes

efriedma added a reviewer: aaron.ballman. efriedma added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2406 + + if (Size.isZero() || Size >= Context.getTypeAlignInChars(EltTy)) +return false; I think you need to check that the size is a multiple o

[PATCH] D133648: Clang, increase upper bound of partially initialized array sizes

efriedma added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:866 ElementEntity.getKind() == InitializedEntity::EK_VectorElement) ElementEntity.setElementIndex(Init); shafik wrote: > `setElementIndex(...)` takes `unsigned` as well and

[PATCH] D133711: [Sema] Reject array element types whose alignments are larger than their sizes

efriedma added a comment. > Usually the solution is to round up the size to the alignment. Can you > provide godbolt examples of what GCC and MSVC do? gcc prints a similar error, MSVC doesn't support an equivalent alignment attribute. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D133711: [Sema] Reject array element types whose alignments are larger than their sizes

efriedma added a comment. > It looks like gcc started rejecting the code relatively recently (11.1). On what example? The code in the commit message gives an error on gcc 5. > Also, I found out that MSVC doesn't reject the following code: This looks like a backwards-compatibility quirk in the

[PATCH] D133711: [Sema] Reject array element types whose alignments are larger than their sizes

efriedma added a comment. Oh, so you mean we need to consider whether we need to avoid emitting an error message when we're dealing with C++ code targeting i686-pc-win32, given we implement the alignment quirk. I think I'd like to try printing an error message, given it's reasonably likely you

[PATCH] D133711: [Sema] Reject array element types whose alignments are larger than their sizes

efriedma added a comment. Messing with the layout is risky. If we give an error, the user will notice something is wrong, and can adapt their code (e.g. explicitly enforcing alignment using alignas). If we silently use a different layout, and the class is used across compilers, the user ends

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

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

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

efriedma added a comment. > No, you can still link those. There's no ABI change nor any interference at > IR level. The scenario I was thinking of with -ffine-grained-bitfield-accesses is something like the following: File A: struct X { int a : 8; int b : 24; }; void f(struct X*); int g

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

efriedma added a comment. > This only matters if the IRs are linked together with IPO. Otherwise, at > object level it doesn't really matter. Right. > Do you think we can get away by just documenting the incompatibility of doing > IPO with files compiled with different -ffine-grained-bitfield-

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

efriedma added a comment. > Is there no way in LLVM to load partially-initialized memory and then freeze > it that will preserve the parts of the value that are initialized? Because > that seems like something that LLVM needs to be providing. It's currently possible to `load <32 x i1>`, freeze

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

efriedma added a comment. > Is that seen as a defect or as for some reason desirable? Because I worry > that optimizer people are talking themselves into something that would be a > truly beautiful model if only there were a frontend that could emit code for > it. > > Doesn't this make it e.g.

[PATCH] D126956: [tbaa] Handle base classes in struct tbaa

efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenTBAA.cpp:366 + // so may differ from declaration order. In particular, Itanium ABI will + // allocate a primary base first. + llvm::sort(Fields, Maybe add a comment explaining why

[PATCH] D122271: [Clang] -Wunused-but-set-variable warning - handle also pre/post unary operators

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

[PATCH] D119493: Fixing surplus assert condition in EvaluateTemporary

efriedma added a comment. I'm not sure this is the right fix. If we were handling the pseudo-destructor correctly, I would expect the following to compile successfully: typedef bool Var; constexpr bool foobool() { return (bool().Var::~Var(), false); } CHANGES SINCE LAST ACTION

[PATCH] D121306: [Sema] add warning for tautological FP compare with literal

efriedma added a comment. In D121306#3405824 , @alexfh wrote: > What's the right fix for this warning, using a literal of the same floating > point type? Probably? Depends on why you're trying to use equality on floating-point numbers; "x == 0.3" is in

[PATCH] D122377: [PowerPC][Linux] Support 16-byte lock free atomics on pwr8 and up

efriedma added inline comments. Comment at: clang/lib/Basic/Targets/PPC.h:451 + MaxAtomicPromoteWidth = 128; + MaxAtomicInlineWidth = 128; +} MaxAtomicPromoteWidth should not depend on whether quadword-atomics is present, only the target OS. It de

[PATCH] D122502: [Clang] Add helper method to determine if a nonvirtual base has an entry in the LLVM struct

efriedma added a comment. I'm not against this, I guess... but clang/lib/CodeGen/CGRecordLayout.h isn't an installed header; it's not clear to me how you're using this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122502/new/ https://reviews.llvm

[PATCH] D122502: [Clang] Add helper method to determine if a nonvirtual base has an entry in the LLVM struct

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

[PATCH] D122377: [PowerPC][Linux] Support 16-byte lock free atomics on pwr8 and up

efriedma added inline comments. Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1329 setOperationAction(ISD::INTRINSIC_VOID, MVT::i128, Custom); } You probably want an "else" here to call `setMaxAtomicSizeInBitsSupported(64)` or something like

[PATCH] D122573: [TBAA] Emit distinct TBAA tags for pointers with different depths,types.

efriedma added a comment. I'm a little skeptical about making TBAA more aggressive. In most situations, we're probably talking about tiny performance gains, and currently there's no good way to check whether a codebase suffers from type punning issues (either with compile-time analysis or runt

[PATCH] D122573: [TBAA] Emit distinct TBAA tags for pointers with different depths,types.

efriedma added a comment. > Maybe we should try improve our tooling to detect violations in parallel to > the effort in this patch? I am planning on trying to resurrect the > type-sanitizer patches. I've also been playing around with a simple tool > that's inserting assertions to check 2 pointe

[PATCH] D122732: [Clang][AArch64][SVE] Allow subscript operator for SVE types

efriedma added a comment. > Subscript on svbool_t is not allowed as this doesn't really have meaningful > semantics. Not sure what you mean by this; LLVM supports extractelement on `` vectors. I guess the fact that it's a "vscale x 16" element vector might not be intuitive? Repository: rG

[PATCH] D130575: clang: include ManagedStatic.h for llvm_shutdown

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

[PATCH] D130574: ClangLinkerWrapper: explicitly #include

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM You don't really need pre-commit review for a patch that just adds an include, though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1

[PATCH] D130576: ManagedStatic: remove from ASTMatchersInternal.h

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

[PATCH] D129118: CommandLine: add and use cl::SubCommand::get{All,TopLevel}

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

[PATCH] D130577: clang-driver: use llvm_fast_shutdown

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

[PATCH] D129131: Remove uses of llvm_shutdown

efriedma added inline comments. Comment at: polly/lib/External/isl/interface/extract_interface.cc:590 delete Clang; - llvm::llvm_shutdown(); nhaehnle wrote: > Meinersbur wrote: > > This file is imported from the upstream project > > (https://repo

[PATCH] D131143: [Clang] Interaction of FP pragmas and function template instantiation

efriedma added a comment. > In the case is explicit instantiation there is apparent connection > between a point in source code and instantiated function. It can > support interaction of pragmas that act in that point. For example, in > the code: > > #pragma STDC FENV_ROUND FE_DOWNWARD > templ

[PATCH] D130907: [Clang] Use pragma FENV_ROUND to reset rounding mode settings.

efriedma added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:398 +// If dynamic rounding mode is specified in absence of FENV_ACCESS, treat it +// as setting default rounding mode. +FpPragmaStack.CurrentValue.clearConstRoundingModeOverride(); -

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

efriedma added a comment. Instead of trying to dig through subexpressions of the ConstantExpr to try to infer the type we need, can we just compute it directly? QualType RetType = CE->getType(); if (CE->isGLValue()) RetType = CGM.getContext().getLValueReferenceType(RetType); Repository

[PATCH] D123627: Correctly diagnose prototype redeclaration errors in C

efriedma added a comment. If the declaration we're redeclaring is a builtin, should the diagnostic be in the "-Wincompatible-library-redeclaration" warning group? With this patch, we treat redefinitions of builtins without a prototype differently from redefinitions with a prototype, for exampl

[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] D106030: [Clang] add support for error+warning fn attrs

efriedma added a comment. In D106030#2878897 , @arsenm wrote: > Adding something to the IR for the sole purpose of producing a diagnostic > feels really weird. I'm not sure I see why the frontend can't see this > attribute and directly warn In C++, you

[PATCH] D104854: Introduce intrinsic llvm.isnan

efriedma added a comment. > The options '-ffast-math' and '-fno-honor-nans' imply that FP operation > operands are never NaNs. This assumption however should not be applied > to the functions that check FP number properties, including 'isnan'. If > such function returns expected result instead of

[PATCH] D94098: [Clang][AArch64] Inline assembly support for the ACLE type 'data512_t'.

efriedma added a comment. In D94098#2886319 , @labrinea wrote: > Firstly, the information that the load/store comes from an inline asm operand > gets lost by the time the SelectionDAG processes those nodes, and so we > cannot use a target hook to select

[PATCH] D105516: [clang][PassManager] Add -falways-mem2reg to run mem2reg at -O0

efriedma added a comment. I think it would be better to focus on making -O1 more usable for this sort of purpose, rather than introduce -O0.5. I mean, there's a lot of wiggle-room between -O0 and -O2, but I don't think it makes sense to add a driver option that promises to run exactly one opti

[PATCH] D105516: [clang][PassManager] Add -falways-mem2reg to run mem2reg at -O0

efriedma added a comment. The part I'm most uncomfortable with is sticking "mem2reg" in a public, documented driver option. I don't want to promise that the mem2reg pass will exist forever. We should be focused on making sure the options we add are stable, and compose effectively, not just be

[PATCH] D106333: [AArch64][SVE] Handle svbool_t VLST <-> VLAT/GNUT conversion

efriedma added a comment. Can you add a helper somewhere for the `BT->getKind() == BuiltinType::SveBool ? getLangOpts().ArmSveVectorBits / getCharWidth() : getLangOpts().ArmSveVectorBits)` pattern? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106

[PATCH] D106333: [AArch64][SVE] Handle svbool_t VLST <-> VLAT/GNUT conversion

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

[PATCH] D106701: [clang] Add -falign-loops=N where N is a power of 2

efriedma added a comment. Can we hook this up to a LLVM IR function attribute, instead of making it a codegen flag? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106701/new/ https://reviews.llvm.org/D106701 ___

[PATCH] D106721: [AArch64] Implemnt MSVC __mulh and __umulh builtins and corresponding IR level intrinsics

efriedma added a comment. Do we need LLVM intrinsics for these? For the x86 equivalents, we just generate `mul i128`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106721/new/ https://reviews.llvm.org/D106721

[PATCH] D103426: Clang: Extend format string checking to wprintf/wscanf

efriedma added a comment. This should split into three patches: 1. The ConvertUTFWrapper patch. 2. The patch to add format warnings for wprintf/wscanf/etc. 3. The patch to add the %l16/%l32 format modifiers. Comment at: clang/lib/AST/Expr.cpp:1091 + if (llvm::convertUTF32

[PATCH] D106721: [AArch64] Implemnt MSVC __mulh and __umulh builtins and corresponding IR level intrinsics

efriedma added a comment. We won't call compiler-rt for i128 multiply on aarch64. It's not worthwhile under any circumstances. Pattern-matching 64/64->128 multiply in SelectionDAG legalization has been reliable in practice, as far as I know. And even it misses due to weird behavior in instco

[PATCH] D106753: ConvertUTF: Created wrapper convertUTF32ToUTF8String

efriedma added a comment. Please fix clang-format issues. (You can use the clang-format-diff.py script.) Comment at: llvm/lib/Support/ConvertUTFWrapper.cpp:154 + // Error out on an uneven byte count. + if (SrcBytes.size() % 2) +return false; Wrong consta

[PATCH] D106753: ConvertUTF: Created wrapper convertUTF32ToUTF8String

efriedma added a comment. The buildbot seems to think your new unittests are broken. Not sure why. (You can run just the unittests with "ninja check-llvm-unit".) Comment at: llvm/include/llvm/Support/ConvertUTF.h:127 +#define UNI_UTF32_BYTE_ORDER_MARK_NATIVE 0xFEFF +#def

[PATCH] D105728: [clang][Codegen] Directly lower `(*((volatile int *)(0))) = 0;` into a `call void @llvm.trap()`

efriedma added a comment. gcc apparently lowers this sort of construct to a volatile load/store followed by a trap. Not sure where the trap is coming from. I'm not sure this solves any problem that really needs to be solved. LLVM isn't going to optimize out a volatile load/store to null. Wel

[PATCH] D104854: Introduce intrinsic llvm.isnan

efriedma added a comment. In D104854#2904826 , @sepavloff wrote: > In D104854#2886328 , @efriedma > wrote: > >>> The options '-ffast-math' and '-fno-honor-nans' imply that FP operation >>> operands are never NaNs

[PATCH] D94098: [Clang][AArch64] Inline assembly support for the ACLE type 'data512_t'.

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

[PATCH] D105516: [clang][PassManager] Add -falways-mem2reg to run mem2reg at -O0

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

[PATCH] D106753: ConvertUTF: Created wrapper convertUTF32ToUTF8String

efriedma added inline comments. Comment at: llvm/lib/Support/ConvertUTFWrapper.cpp:176 + // enough that we can fit a null terminator without reallocating. + Out.resize(SrcBytes.size() * UNI_MAX_UTF8_BYTES_PER_CODE_POINT + 1); + UTF8 *Dst = reinterpret_cast(&Out[0]); --

[PATCH] D106959: [PowerPC] swdiv builtins for XL compatibility

efriedma added inline comments. Comment at: llvm/test/CodeGen/PowerPC/LowerCheckedFPArith.ll:36 +; CHECK-NEXT: %2 = fdiv fast float %0, %1 +; CHECK-NEXT: %3 = fcmp une float %2, %2 +; CHECK-NEXT: br i1 %3, label %swdiv_HWDIV, label %swdiv_MERGE A "fast" fdiv n

[PATCH] D120936: [Sema][Windows] Don't special-case void* in __unaligned conversions.

efriedma updated this revision to Diff 413989. efriedma added a comment. Added warning. This roughly matches the corresponding MSVC warning, as far as I can tell. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120936/new/ https://reviews.llvm.org/

[PATCH] D120936: [Sema][Windows] Don't special-case void* in __unaligned conversions.

efriedma added a comment. In D120936#3368566 , @rnk wrote: > I wasn't able to find any history for why `void*` was special here. As far as I can tell, nobody ever thought `void*` was special. The check was an attempt to restrict dubious conversions whi

[PATCH] D121412: Complete the list of single-underscore keywords for MSVC compat.

efriedma created this revision. efriedma added reviewers: rnk, mstorsjo. Herald added a subscriber: dexonsmith. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. List derived from https://docs.microsoft.com/en-us/cpp/cpp/keywords-cpp . Not th

[PATCH] D121412: Complete the list of single-underscore keywords for MSVC compat.

efriedma updated this revision to Diff 414499. efriedma added a comment. Drop _except; __except is a contextual keyword. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121412/new/ https://reviews.llvm.org/D121412 Files: clang/include/clang/Basic/

[PATCH] D120936: [Sema][Windows] Don't special-case void* in __unaligned conversions.

This revision was automatically updated to reflect the committed changes. Closed by commit rG27a574962567: [Sema][Windows] Don't special-case void* in __unaligned conversions. (authored by efriedma). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1209

[PATCH] D121412: Complete the list of single-underscore keywords for MSVC compat.

efriedma added a comment. That makes sense, sure. -fms-compatibility is on by default for clang-cl anyway. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121412/new/ https://reviews.llvm.org/D121412 ___

[PATCH] D121412: Complete the list of single-underscore keywords for MSVC compat.

efriedma updated this revision to Diff 415136. efriedma added a comment. Use KEYMSCOMPAT Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121412/new/ https://reviews.llvm.org/D121412 Files: clang/include/clang/Basic/TokenKinds.def Index: clang/in

[PATCH] D121715: [Clang] Fix an unused-but-set-variable warning with compond assignment

efriedma added inline comments. Comment at: clang/test/Sema/warn-unused-but-set-variables.c:66 + for (int i = 0; i < 1000; i++) +j += 1; + return; The handling of this testcase without your patch seems fine. Even if the value of j is technically read, it'

[PATCH] D121412: Complete the list of single-underscore keywords for MSVC compat.

This revision was automatically updated to reflect the committed changes. Closed by commit rG5791e28f3016: Complete the list of single-underscore keywords for MSVC compat. (authored by efriedma). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121412/n

[PATCH] D121865: [CodeGen] Inline _byteswap_* builtins.

efriedma created this revision. efriedma added reviewers: rnk, mstorsjo. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. As discussed in D57915 . Fixes https://github.com/llvm/llvm-project/issues/3 . Re

[PATCH] D121865: [CodeGen] Inline _byteswap_* builtins.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG04ba344176b2: [CodeGen] Inline _byteswap_* builtins. (authored by efriedma). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

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

efriedma added a comment. Herald added a project: All. D120026 is merged now, which addresses the issue of the compiler generating __sync calls where it isn't supposed to. Does anyone want to continue discussing what changes to compiler-rt would be appropriate

[PATCH] D121715: [Clang] Fix an unused-but-set-variable warning with volatile variable

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/Sema/SemaDecl.cpp:2033 return; + unsigned DiagID = isa(VD) ? diag::warn_unused_but_set_parameter (Accidental change?)

[PATCH] D121715: [Clang] Fix an unused-but-set-variable warning with volatile variable

efriedma added a comment. In D121715#3397215 , @xbolva00 wrote: > If there is j = j + 1 .. do we warn ? In D121715#3397237 , @xbolva00 wrote: > j++; > > Should not we warn in this case? (Currently no warning

[PATCH] D121715: [Clang] Fix an unused-but-set-variable warning with volatile variable

efriedma accepted this revision. efriedma added a comment. In D121715#3397446 , @yonghong-song wrote: > The 'j++' is not warned as it is not a BinaryOperator or CXXOperatorCallExpr > so it is not handled and considered no warning by default. > This patc

[PATCH] D123642: [clang codegen] Assume arguments of __atomic_* are aligned.

efriedma created this revision. efriedma added reviewers: jyknight, craig.topper, jfb, rjmccall. Herald added a subscriber: StephenFan. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. This seems weird, but it's necessary to match gcc, and avo

[PATCH] D123648: Restrict lvalue-to-rvalue conversions in CGExprConstant.

efriedma created this revision. efriedma added reviewers: aaron.ballman, rjmccall, rsmith. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. We were generating wrong code for cxx20-consteval-crash.cpp: instead of loading a value of a variable,

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

efriedma created this revision. efriedma added reviewers: aaron.ballman, rsmith, erichkeane. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. Flexible array initialization is a C/C++ extension implemented in many compilers to allow initializi

[PATCH] D123680: Add support for ignored bitfield conditional codegen.

efriedma added a comment. This is basically what I expected; the general approach seems fine. Comment at: clang/lib/CodeGen/CGExpr.cpp:200 + E->IgnoreParenNoopCasts(getContext( { +if (CondOp->getObjectKind() == OK_BitField) + return EmitIgnoredConditionalO

[PATCH] D123680: Add support for ignored bitfield conditional codegen.

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

[PATCH] D123648: Restrict lvalue-to-rvalue conversions in CGExprConstant.

efriedma added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1096 +case CK_LValueToRValue: { + // We don't really support doing lvalue-to-rvalue conversions here; any + // interesting conversions should be done in Evaluate(). But as a --

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

efriedma added inline comments. Comment at: clang/test/SemaCXX/constant-expression-cxx11.cpp:2388 + // evaluation. Make sure we emit a sane error message, for now. + constexpr A c = {1, 2, 3}; // expected-warning {{flexible array initialization is a GNU extension}} + static_a

[PATCH] D123648: Restrict lvalue-to-rvalue conversions in CGExprConstant.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd791de0e25e1: Restrict lvalue-to-rvalue conversions in CGExprConstant. (authored by efriedma). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D123642: [clang codegen] Assume arguments of __atomic_* are aligned.

efriedma added a comment. > I disagree with this on principle -- IMO, it is basically a historical bug in > GCC that it ignores the type alignment, and we should NOT try to match that > -- it's dangerous. gcc has always behaved this way, and I don't see any indication they intend to change it.

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

efriedma updated this revision to Diff 422671. efriedma added a comment. Address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123649/new/ https://reviews.llvm.org/D123649 Files: clang/include/clang/AST/Decl.h clang/include/cla

[PATCH] D123627: Correctly diagnose prototype redeclaration errors in C

efriedma added a comment. In D123627#3451373 , @aaron.ballman wrote: > Thank you for letting me know -- I've speculatively fixed the issue in > 726901d06aab2f92d684d28507711308368c29d6 >

[PATCH] D123627: Correctly diagnose prototype redeclaration errors in C

efriedma added a comment. > The reason you get the weird behavior with the note pointing to the same line > as the declaration is because rintf() is a predefined builtin: > https://godbolt.org/z/j3W759M7a (note the same lovely diagnostic note > behavior). It points at the same location, but at

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG5955a0f9375a: Allow flexible array initialization in C++. (authored by efriedma). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

efriedma added a comment. Apparently the assertions I added are triggering in the LLVM test-suite. Commented them out in 6cf0b1b3da3e8662baf030a2d741e3becaaab2b0 ; I'll come up with a proper fix soon. Repository: rG LLVM

[PATCH] D123826: Fix size of flexible array initializers, and re-enable assertions.

efriedma created this revision. efriedma added a reviewer: erichkeane. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. In D123649 , I got the formula for getFlexibleArrayInitChars slightly wrong: the flexibl

[PATCH] D123826: Fix size of flexible array initializers, and re-enable assertions.

efriedma added inline comments. Comment at: clang/lib/AST/Decl.cpp:2735 + const Expr *FlexibleInit = List->getInit(List->getNumInits() - 1); + auto InitTy = Ctx.getAsConstantArrayType(FlexibleInit->getType()); + if (!InitTy) erichkeane wrote: > Same here You w

[PATCH] D123642: [clang codegen] Assume arguments of __atomic_* are aligned.

efriedma added a comment. > Have we verified that this is the rule that GCC uses? Is it true even if e.g. > the pointer expression is the address of a variable with a known alignment, > or if the pointer has an explicitly-aligned type (e.g. with an aligned > typedef)? As far as I can tell, if

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

efriedma added a comment. 6cf0b1b3 was a temporary fix to stop the crash. D123826 is the full fix to make the assertion actually work correctly. Repository: rG LLVM Github Monorepo CHAN

[PATCH] D123826: Fix size of flexible array initializers, and re-enable assertions.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG4802edd1ac7a: Fix size of flexible array initializers, and re-enable assertions. (authored by efriedma). Changed prior to commit: https://reviews.

[PATCH] D123642: [clang codegen] Assume arguments of __atomic_* are aligned.

efriedma abandoned this revision. efriedma added a comment. Opened https://github.com/llvm/llvm-project/issues/54963 to more specifically track the clang issue. With b27430f , it should be easy enough to special-case calls to

[PATCH] D123950: Look through calls to std::addressof to compute pointer alignment.

efriedma created this revision. efriedma added reviewers: jyknight, rsmith, rjmccall. Herald added a project: All. efriedma requested review of this revision. Herald added a project: clang. This is sort of a followup to D37310 ; that basically fixed the same issue

[PATCH] D124038: [clang] Prevent folding of non-const compound expr

efriedma added a comment. The fix doesn't look right. A CompoundLiteralExpr is itself an lvalue; we should catch any issues when we visit it, not a VarDecl that contains a pointer to it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124038/new/ h

[PATCH] D124038: [clang] Prevent folding of non-const compound expr

efriedma added a comment. The `VD->getAnyInitializer()->IgnoreCasts()` is the part that's wrong. We want to allow something like the following: constexpr int *q = (int *)(int[1]){3}; constexpr int *q2 = q; To catch the bad cases specifically, we have to wait until we actually reach the co

<    3   4   5   6   7   8   9   10   11   12   >